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

add test for a 1-block reorg in short_sync_backtrack() #18966

Open
wants to merge 1 commit into
base: short_sync_backtrack_fix
Choose a base branch
from

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Dec 3, 2024

Purpose:

Add test coverage for the fixed bug. This test fails when run on top of main, but passes with Almog's fix.

Current Behavior:

1-block reorg in short_sync_backtrack() is not covered by a test.

New Behavior:

1-block reorg in short_sync_backtrack() is covered by a test.

@arvidn arvidn added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Tests Changes to tests labels Dec 3, 2024
@arvidn arvidn marked this pull request as ready for review December 3, 2024 12:24
@arvidn arvidn requested a review from a team as a code owner December 3, 2024 12:24

# validate reward coins
for reward in rewards:
rec = records.pop(reward.name())
Copy link
Contributor

Choose a reason for hiding this comment

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

what was the purpose of moving this here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remove the records from the database that has been accounted for from the actual block in order to ensure the list is empty at the end. Otherwise I couldn't detect additional coin entries in the database, that don't exist in the block.

)

print(f"chain A: {chain_a[-1].header_hash.hex()}")
await add_blocks_in_batches(chain_a[-1:], full_node_1.full_node, chain[-1].header_hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

i think chain_a[-1] instead of chain_a[-1:] since the intention is to only add one block here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but add_blocks_in_batches takes a list of blocks, not just a single one

Copy link
Contributor

Choose a reason for hiding this comment

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

oh right, ok i think [chain_a[-1]] would maybe be more clear for someone who is not familiar with what we are doing

chain_b = bt.get_consecutive_blocks(
1,
chain,
guarantee_transaction_block=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

theres a better way to do this instead of randomly trying, you can enforce the sp the block would be created after, so you can make sure the chain_a would be infused after the chain_b block, you can also farm it on a new slot entirely that would have the same effect

check out

# make two new blocks on tip, block_2 has higher total iterations

Copy link
Contributor

github-actions bot commented Dec 4, 2024

File Coverage Missing Lines
chia/_tests/core/full_node/test_full_node.py 96.7% lines 2612-2613
Total Missing Coverage
61 lines 2 lines 96%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog coverage-diff Tests Changes to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants