refactor: Remove nChainTx from consensus #29349

1. 
   maflcko commented at 5:08 pm on January 30, 2024: member

   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.

2. 
   DrahtBot commented at 5:08 pm on January 30, 2024: contributor

   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.

3. DrahtBot renamed this:
   refactor: Remove nChainTx from consensus
   refactor: Remove nChainTx from consensus
   on Jan 30, 2024
4. DrahtBot added the label Refactoring on Jan 30, 2024
5. in src/chain.h:95 in fa4ee23ecd outdated

    97-     * sigops, size, merkle root. Implies all parents are at least TREE but not necessarily TRANSACTIONS. When all
    98-     * parent blocks also have TRANSACTIONS, CBlockIndex::nChainTx will be set.
    99+     * sigops, size, merkle root. Implies all parents are at least TREE but not necessarily TRANSACTIONS.
   100      */
   101-    BLOCK_VALID_TRANSACTIONS =    3,
   102+    BLOCK_VALID_TRANSACTIONS =    2,
   

   ---

   ---

   

   sdaftuar commented at 6:15 pm on January 30, 2024:

   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?

   ---

   

   maflcko commented at 7:16 pm on January 30, 2024:

   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.

   ---

   

   maflcko commented at 5:07 pm on February 2, 2024:

   Fixed. (Implemented my suggestion)
6. maflcko marked this as a draft on Jan 30, 2024
7. refactor: Remove default BlockStatus in IsValid check fa0a3c3801
8. maflcko renamed this:
   refactor: Remove nChainTx from consensus
   CBlockIndex: Remove nChainTx from consensus
   on Feb 2, 2024
9. maflcko removed the label Refactoring on Feb 2, 2024
10. maflcko added the label UTXO Db and Indexes on Feb 2, 2024
11. maflcko removed the label UTXO Db and Indexes on Feb 2, 2024
12. maflcko added the label Refactoring on Feb 2, 2024
13. maflcko renamed this:
    CBlockIndex: Remove nChainTx from consensus
    refactor: Remove nChainTx from consensus
    on Feb 2, 2024
14. refactor: Remove nChainTx from consensus 6cf0386a01
15. refactor: Use BLOCK_VALID_TRANSACTIONS_TREE over nChainTx in net_processing 6670123539
16. maflcko force-pushed on Feb 2, 2024
17. 
    maflcko commented at 1:14 pm on February 5, 2024: member

    Closing for now, but happy to reopen if someone thinks this is useful.
18. maflcko closed this on Feb 5, 2024

    ---

19. maflcko deleted the branch on Feb 5, 2024
20. bitcoin locked this on Feb 4, 2025

Contributors
maflcko DrahtBot sdaftuar

Labels
Refactoring