ConnectBlock: don’t serialize block hash twice #23819

1. 
   jb55 commented at 9:06 pm on December 19, 2021: member

   In the validation:block_connected tracepoint, we call block->GetHash(), which ends up calling CBlockHeader::GetHash(), executing around 8000 serialization instructions. We don’t need to do this extra work, because block->GetHash() is already called further up in the function. Let’s save that value as a local variable and re-use it in our tracepoint so there is no unnecessary tracepoint overhead.

   Shave off an extra 100 or so instructions from the validation:block_connected tracepoint by reusing a nearby GetTimeMicros(). This brings the tracepoint down to 54 instructions. Still high, but much better than the previous ~154 and 8000 instructions which it was originally.

   Signed-off-by: William Casarin jb55@jb55.com

2. 
   jb55 commented at 9:09 pm on December 19, 2021: member

   cc @laanwj @0xB10C
3. DrahtBot added the label Validation on Dec 19, 2021
4. 
   0xB10C commented at 11:48 am on December 20, 2021: member

   Concept ACK, will review in the next days.

   ref: #23724

5. theStack approved
6. 
   theStack commented at 9:55 pm on January 1, 2022: member

   Code-review ACK b60c8fdc0772e78cb4c8b937f35afb3e25d3df56

   Not related to the tracepoint overhead, but maybe still worth mentioning: note that in the PR branch, block.GetHash() is still called more than once in CChainState::ConnectBlock, as there is another invocation at the very start: https://github.com/bitcoin/bitcoin/blob/a1e04e10799791a728cce4430808517598c8742c/src/validation.cpp#L1869 (As far as I know, we enforce compiling with assertions on, i.e. NDEBUG never being set)

7. jb55 force-pushed on Jan 4, 2022
8. 
   jb55 commented at 5:04 pm on January 4, 2022: member

   On Sat, Jan 01, 2022 at 01:55:29PM -0800, Sebastian Falbesoner wrote:

   Not related to the tracepoint overhead, but maybe still worth mentioning: note that in the PR branch, block.GetHash() is still called more than once in CChainState::ConnectBlock, as there is another invocation at the very start: https://github.com/bitcoin/bitcoin/blob/a1e04e10799791a728cce4430808517598c8742c/src/validation.cpp#L1869 (As far as I know, we enforce compiling with assertions on, i.e. NDEBUG never being set)

   great catch. I’ve moved the blockHash variable to above the assert, so we shave off ~16,000 instructions now.

9. theStack approved
10. 
    theStack commented at 6:57 pm on January 4, 2022: member

    Code review re-ACK 8f8b1969e9d15117fe89ecc377d83e3f7b8dc5c1

    Slightly off-topic, but could you elaborate how exactly you measure the number of saved instructions (and on which architecture)? Do you statically analyse the compiled binary or do it dynamically via perf or alike?

11. 
    jb55 commented at 7:05 pm on January 4, 2022: member

    On Tue, Jan 04, 2022 at 10:57:28AM -0800, Sebastian Falbesoner wrote:

    Slightly off-topic, but could you elaborate how exactly you measure the number of saved instructions (and on which architecture)? Do you statically analyse the compiled binary or do it dynamically via perf or alike?

    See #23724 (comment) for the method and #23724 (comment) for where I determined that GetHash hits 8000 instructions.

12. fanquake requested review from laanwj on Jan 5, 2022
13. fanquake referenced this in commit 542e405a85 on Jan 10, 2022
14. sidhujag referenced this in commit 5ca60a8fdc on Jan 10, 2022
15. luke-jr approved
16. 
    luke-jr commented at 11:11 pm on January 11, 2022: member

    utACK

    I’m surprised we’re not caching block hashes like we do for transactions, though?

17. 
    jb55 commented at 0:55 am on January 12, 2022: member

    On Tue, Jan 11, 2022 at 03:11:55PM -0800, Luke Dashjr wrote:

    I’m surprised we’re not caching block hashes like we do for transactions, though?

    Where is it cached for transactions? I see CMutableTransaction::GetHash but it looks roughly the same as CBlockHeader::GetHash.

18. 
    luke-jr commented at 1:02 am on January 12, 2022: member

    Check CTransaction
19. 
    jb55 commented at 2:39 am on January 12, 2022: member

    On Tue, Jan 11, 2022 at 05:03:06PM -0800, Luke Dashjr wrote:

    Check CTransaction

    gotcha, I see ComputeHash in the constructor. Yes it looks like CBlockHeader doesn’t have caching, I’m guessing because it’s a mutable thing?

20. MarcoFalke renamed this:
    tracing/block_connected: don't serialize block hash twice
    ConnectBlock: don't serialize block hash twice
    on Jan 12, 2022
21. MarcoFalke added the label Refactoring on Jan 12, 2022
22. 
    MarcoFalke commented at 7:48 am on January 12, 2022: member

    Looks like this is also refactoring ConnectBlock, which is not tracing related? -> Renamed pull title
23. in src/validation.cpp:1870 in 8f8b1969e9 outdated

    1865@@ -1866,7 +1866,10 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
    1866 {
    1867     AssertLockHeld(cs_main);
    1868     assert(pindex);
    1869-    assert(*pindex->phashBlock == block.GetHash());
    1870+
    1871+    uint256 blockHash = block.GetHash();
    

    ---

    ---

    

    MarcoFalke commented at 7:51 am on January 12, 2022:

    0    const uint256 block_hash{block.GetHash()};
    

    style nit

24. MarcoFalke approved
25. 
    MarcoFalke commented at 7:52 am on January 12, 2022: member

    lgtm. Left a style nit
26. block_connected: don't serialize block hash twice

    In the validation:block_connected tracepoint, we call block->GetHash(),
    which ends up calling CBlockHeader::GetHash(), executing around 8000
    serialization instructions. We don't need to do this extra work, because
    block->GetHash() is already called further up in the function. Let's
    save that value as a local variable and re-use it in our tracepoint so
    there is no unnecessary tracepoint overhead.
    
    Signed-off-by: William Casarin <jb55@jb55.com>

    80e1c55687
27. block_connected: re-use previous GetTimeMicros

    Shave off an extra 100 or so instructions from the
    validation:block_connected tracepoint by reusing a nearby
    GetTimeMicros(). This brings the tracepoint down to 54 instructions.
    Still high, but much better than the previous ~154 and 8000 instructions
    which it was originally.
    
    Signed-off-by: William Casarin <jb55@jb55.com>

    eb8b22d517
28. jb55 force-pushed on Jan 12, 2022
29. theStack approved
30. 
    theStack commented at 5:32 pm on January 12, 2022: member

    re-ACK eb8b22d5176d7abc6f93b4473df446105ca595e6
31. 
    0xB10C commented at 11:15 am on January 13, 2022: member

    ACK eb8b22d5176d7abc6f93b4473df446105ca595e6

    I’m measuring 7 instructions. Code changes look good to me. Still needs refactoring in the PR title :)

     0(gdb) b src/validation:2161
     1Breakpoint 1 at 0x2c57b0: file src/validation.cpp, line 2161.
     2(gdb) b src/validation:
     3Breakpoint 2 at 0x2c57de: file src/validation.cpp, line 2172.
     4(gdb) run
     5[..]
     6
     7Thread 22 "b-msghand" hit Breakpoint 1, CChainState::ConnectBlock (this=0x555555d19980, block=..., state=...,
     8    pindex=<optimized out>, view=..., fJustCheck=false) at validation.cpp:2161
     92161	    LogPrint(BCLog::BENCH, "    - Index writing: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5 - nTime4), nTimeIndex * MICRO, nTimeIndex * MILLI / nBlocksTotal);
    10(gdb) n
    112163	    TRACE6(validation, block_connected,
    12(gdb) record btrace
    13(gdb) c
    14Continuing.
    15
    16Thread 22 "b-msghand" hit Breakpoint 2, CCheckQueueControl<CScriptCheck>::~CCheckQueueControl (
    17    this=<optimized out>, __in_chrg=<optimized out>) at validation.cpp:2172
    182172	    return true;
    19(gdb) record info
    20Undefined record command: "info".  Try "help record".
    21(gdb) info record
    22[..]
    23Recorded 7 instructions in 1 functions (0 gaps) for thread 22 (Thread 0x7fff2e7fc640 (LWP 508792)).
    

    @jb55 you mention 54 instructions in eb8b22d5176d7abc6f93b4473df446105ca595e6. I think your measurements include the instructions from line 2161. I’m starting the measurement in line 2163.

    https://github.com/bitcoin/bitcoin/blob/eb8b22d5176d7abc6f93b4473df446105ca595e6/src/validation.cpp#L2161-L2172

32. fanquake added this to the milestone 23.0 on Jan 18, 2022
33. 
    laanwj commented at 1:09 pm on February 17, 2022: member

    Code review ACK eb8b22d5176d7abc6f93b4473df446105ca595e6
34. laanwj merged this on Feb 17, 2022
35. laanwj closed this on Feb 17, 2022

    ---

36. sidhujag referenced this in commit 504a590bf7 on Feb 18, 2022
37. PastaPastaPasta referenced this in commit 70717c901d on Oct 16, 2022
38. Fabcien referenced this in commit c3ed96a41c on Nov 30, 2022
39. DrahtBot locked this on Feb 17, 2023

Contributors
jb55 0xB10C theStack luke-jr MarcoFalke laanwj

Review Requested
laanwj

Labels
Refactoring Validation

Milestone
23.0