index: coinstats reorg, fail when block cannot be reversed #28427

1. 
   furszy commented at 3:17 pm on September 7, 2023: member

   Found it while reviewing #24230 (review).

   During a reorg, continuing execution when a block cannot be reversed leaves the coinstats index in an inconsistent state. This was surely overlooked when ‘CustomRewind’ was implemented.

2. index: coinstats reorg, fail when block cannot be reversed

   During a reorg, continuing execution when a block cannot be
   reversed leaves the coinstats index in an inconsistent state,
   which was surely overlooked when 'CustomRewind' was implemented.

   eef595560e
3. 
   DrahtBot commented at 3:17 pm on September 7, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	ryanofsky
   Stale ACK	TheCharlatan

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #26966 (index: blockfilter initial sync speedup, parallelize process by furszy)
   • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

   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.

4. DrahtBot added the label UTXO Db and Indexes on Sep 7, 2023
5. 
   fanquake commented at 3:23 pm on September 7, 2023: member

   cc @ryanofsky @mzumsande
6. ryanofsky approved
7. 
   ryanofsky commented at 4:29 pm on September 7, 2023: contributor

   Code review ACK eef595560e9ecf3a0d1db4d8ea7ecc33a49d839f

   This seems like it should avoid corrupting the index if ReverseBlock fails, since it avoid writing the best block. It also could prevent assert failures because it avoids subsequent ReverseBlock calls.

   It would be good to add [[nodiscard]] to Reverseblock and other functions in indexing code to prevent this problem from happening other places. Grepping a little for functions in src/index/ that write to the database and return bool I see TxIndex::DB::WriteTxs and CopyHeightIndexToHashIndex and there may be others.

8. TheCharlatan approved
9. 
   TheCharlatan commented at 8:58 am on September 8, 2023: contributor

   ACK eef595560e9ecf3a0d1db4d8ea7ecc33a49d839f

   As said, adding nodiscard would be nice.

10. 
    fanquake commented at 9:16 am on September 8, 2023: member

    It would be good to add [[nodiscard]] to Reverseblock and other functions in indexing code to prevent this problem from happening other places. Grepping a little for functions in src/index/ that write to the database and return bool I see TxIndex::DB::WriteTxs and CopyHeightIndexToHashIndex and there may be others.

    As said, adding nodiscard would be nice. @furszy do you want to push an additional commit?

11. index: add [nodiscard] attribute to functions writing to the db c0bf667912
12. 
    furszy commented at 1:07 pm on September 8, 2023: member

    It would be good to add [[nodiscard]] to Reverseblock and other functions in indexing code to prevent this problem from happening other places. Grepping a little for functions in src/index/ that write to the database and return bool I see TxIndex::DB::WriteTxs and CopyHeightIndexToHashIndex and there may be others.

    As said, adding nodiscard would be nice.

    @furszy do you want to push an additional commit?

    yep, pushed. A bit late due time zone diff.

    Thanks both!

13. maflcko added this to the milestone 26.0 on Sep 10, 2023
14. ryanofsky approved
15. 
    ryanofsky commented at 4:49 pm on September 11, 2023: contributor

    Code review ACK c0bf667912064960df194ea94150976b34f7c267. Only change since last review is new commit adding [[nodiscard]]
16. DrahtBot requested review from TheCharlatan on Sep 11, 2023
17. fanquake merged this on Sep 12, 2023
18. fanquake closed this on Sep 12, 2023

    ---

19. luke-jr referenced this in commit ddf30ef3f9 on Sep 16, 2023
20. Frank-GER referenced this in commit 5f3d90b597 on Sep 19, 2023
21. bitcoin locked this on Sep 11, 2024

Contributors
furszy DrahtBot fanquake ryanofsky TheCharlatan

Review Requested
TheCharlatan

Labels
UTXO Db and Indexes

Milestone
26.0