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: Remove nChainTx from consensus #29349

Closed
wants to merge 3 commits into from

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jan 30, 2024

nChainTx is returned by RPCs and is used to estimate (sync and rescan) progress.

However, it is also used for consensus and P2P logic to denote that a block and all its parents have BLOCK_VALID_TRANSACTIONS set.

This is confusing, because:

  • It would be clearer to use the BlockStatus enum for this as well.
  • It requires AssumeUtxo to fake the nChainTx values.
  • It makes it harder to remove nChainTx completely, or limit it to code that needs it (estimate sync and scan progress and report it on RPC).

Fix it by introducing a new BLOCK_VALID_TRANSACTIONS_TREE level and use it where possible.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 30, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29370 (assumeutxo: Get rid of faked nTx and nChainTx values by ryanofsky)
  • #29355 (doc: Assert that assumed-valid blocks are not fully valid in CheckBlockIndex() by maflcko)
  • #28120 (p2p: make block download logic aware of limited peers threshold by furszy)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot changed the title refactor: Remove nChainTx from consensus refactor: Remove nChainTx from consensus Jan 30, 2024
src/chain.h Outdated
*/
BLOCK_VALID_TRANSACTIONS = 3,
BLOCK_VALID_TRANSACTIONS = 2,
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to understand if it's safe to modify these values -- I believe we write the nStatus field to disk and load it on restart, so wouldn't changing these values require a block-index-upgrade mechanism to be created, in order to avoid problems on restart?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. It should be possible to keep the persisted format by using a serialize helper that translates nStatus both ways losslessly and then recover BLOCK_VALID_TRANSACTIONS_TREE in LoadBlockIndex as a memory-only value.

Though, I wonder if this pull request is still worth it, if more code changes are needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. (Implemented my suggestion)

@maflcko maflcko changed the title refactor: Remove nChainTx from consensus CBlockIndex: Remove nChainTx from consensus Feb 2, 2024
@maflcko maflcko changed the title CBlockIndex: Remove nChainTx from consensus refactor: Remove nChainTx from consensus Feb 2, 2024
@maflcko
Copy link
Member Author

maflcko commented Feb 5, 2024

Closing for now, but happy to reopen if someone thinks this is useful.

@maflcko maflcko closed this Feb 5, 2024
@maflcko maflcko deleted the 2401-less_nChainTx- branch February 5, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants