cryptarchia: fix try_create_fork to find parent block #84
Conversation
cryptarchia/cryptarchia.py
Outdated
| if chain.contains_block(block): | ||
| block_position = chain.block_position(block) | ||
| block_position = chain.block_position(block.parent) | ||
| if block_position != -1: | ||
| return Chain( | ||
| blocks=chain.blocks[:block_position], | ||
| blocks=chain.blocks[: block_position + 1], |
There was a problem hiding this comment.
nice catch, our tests weren't catching this? 😨
I was actually just writing some simulation tests over the weekend for the new total stake inference protocol and saw that different nodes ended up with a different number of LedgerStates at the end of the simulation. I didn't get to debugging that yet but I think this bug might be why.
There was a problem hiding this comment.
@davidrusu @zeegomo I just added a test that covers all cases of fork creation. 2460827
Previously, this case hasn't been caught by tests because we've only tested the case which a fork is created from the genesis block, not from the middle of the chain.
With the previous implementation, the test I added is failed as below:
test_fork_creation (cryptarchia.test_ledger_state_update.TestLedgerStateUpdate.test_fork_creation) ... FAIL
======================================================================
FAIL: test_fork_creation (cryptarchia.test_ledger_state_update.TestLedgerStateUpdate.test_fork_creation)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/rei/repos/nomos-specs/cryptarchia/test_ledger_state_update.py", line 112, in test_fork_creation
assert len(follower.forks) == 2, f"{len(follower.forks)}"
AssertionError: 1
----------------------------------------------------------------------
Ran 1 test in 0.001s
FAILED (failures=1)
Sure. I'll add tests in this PR soon and re-request your review! |
|
Good catch, concerning that it was not caught by tests |
youngjoon-lee
force-pushed
the
cryptarchia-fix-fork-creation
branch
from
792316f to
2460827
Compare
It seems that we should find the position of the "parent" of the block to be added when trying to create a fork, but the current implementation finds the position of the "block" to be added.
Please correct me if I'm wrong.