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

refactor full_node add_block #18584

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

refactor full_node add_block #18584

wants to merge 12 commits into from

Conversation

almogdepaz
Copy link
Contributor

Purpose:

full node add_block is only used to add blocks on top of the current Peak, it does not need ForkInfo besides from the usage in tests, this pr removes that field and updates all the tests, this allows us to remove cases from blockchain.py add_block that dont serve us in node operation

Current Behavior:

New Behavior:

Testing Notes:

@almogdepaz almogdepaz added Blockchain team Issues tagged for Blockchain team to work on full_node Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog Cleanup Code cleanup labels Sep 12, 2024
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

looks really good!

chia/consensus/blockchain.py Outdated Show resolved Hide resolved
chia/consensus/blockchain.py Outdated Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
chia/_tests/blockchain/test_blockchain.py Outdated Show resolved Hide resolved
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Sep 26, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Sep 29, 2024
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Oct 1, 2024
Copy link
Contributor

github-actions bot commented Oct 1, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@almogdepaz
Copy link
Contributor Author

almogdepaz commented Oct 10, 2024

@arvid
from what i can tell theres an issue where a wallet with a trusted node will lose information if the node and wallet where considered synced and the node went into a long sync

there are 2 options for updating the wallet in this case from what i can tell

  1. send all the updates to the wallet when we are done -> we dont currently do that when we long sync, we dont aggregrate each batch result and the wallet wont get the updates at the end this
    state_change_summary = StateChangeSummary(peak, uint32(max(peak.height - 1, 0)), [], [], [], [])
    is where we stage the summary that we send to update wallets inside peak_post_processing_2
  2. the wallet will trigger a wallet long sync and ask for everything it needs -> this only happens when the node is marked as not_synced in the wallet but if it was already marked as synced the only way for it to change is a disconnect

still trying to understand whats the better path here, if its aggregating or just telling the wallet to resync

i assume telling the wallet to resync since there is a limit to how much data we can aggregate and send in one go

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

1 similar comment
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Oct 15, 2024
@almogdepaz almogdepaz marked this pull request as ready for review October 15, 2024 14:47
@almogdepaz almogdepaz requested a review from a team as a code owner October 15, 2024 14:47
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Oct 16, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Oct 16, 2024
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Oct 16, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Oct 16, 2024
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

looks really good. I have a few questions. Was it deliberate to add the __init__.py file?

chia/_tests/process_junit.py Show resolved Hide resolved
chia/_tests/util/misc.py Show resolved Hide resolved
chia/consensus/blockchain.py Show resolved Hide resolved
chia/consensus/blockchain.py Show resolved Hide resolved
chia/full_node/full_node.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blockchain team Issues tagged for Blockchain team to work on Cleanup Code cleanup Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog full_node
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants