Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(consensus): Assert that consensus ended in a valid state after processing a new block #1176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions hathor/consensus/block_consensus.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,19 @@ def log(cls) -> Any:

def update_consensus(self, block: Block) -> None:
self.update_voided_info(block)
self.assert_valid_consensus(block)

def assert_valid_consensus(self, block: Block) -> None:
"""Assert that all transactions confirmed by the block are valid.

This assertion might be slow because it runs in O(n) where n is number of transactions confirmed
by the block."""
Comment on lines +55 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should start running some nodes with -O. It removes assert statements, so we could have more liberty in adding costly assertions. We should keep some nodes without -O so we do catch bugs when those asserts are reached, but some other nodes could run without them so they don't have a performance penalty.

*The loop, or the whole function, would have to be moved to if __debug__.

for tx in block.iter_transactions_in_this_block():
# Since this is the last step in the consensus, the conflict resolution has already been resolved.
# Hence, at this point, all transactins confirmed by a block must be non-voided.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: transactins -> transactions

if tx_meta.voided_by and bool(self.context.consensus.soft_voided_tx_ids & tx_meta.voided_by):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there are other IDs in voided_by that are not soft_voided? Shouldn't it be invalid?

continue
assert tx_meta.voided_by is None

def update_voided_info(self, block: Block) -> None:
""" This method is called only once when a new block arrives.
Expand Down
2 changes: 2 additions & 0 deletions tests/simulation/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ def setUp(self) -> None:
self.simulator = Simulator(self.seed_config)
self.simulator.start()

self.reactor = self.simulator._clock

print('-'*30)
print('Simulation seed config:', self.simulator.seed)
print('-'*30)
Expand Down
Loading