Skip witness sighash cache for non-segwit transactions #9572

1. 
   jl2012 commented at 7:55 am on January 18, 2017: contributor

   This saves unnecessary hash caching for non-segwit transactions, but I am not sure if the difference is noticeable.
2. jl2012 force-pushed on Jan 18, 2017
3. jonasschnelli added the label Refactoring on Jan 18, 2017
4. 
   sdaftuar commented at 6:29 pm on January 18, 2017: member

   Concept ACK (I vaguely recall from when we introduced PrecomputedTransactionData that it was non-negligible performance impact, I can re-check).

   I’m not sure about this approach though, because SignatureHash assumes that if the cache passed in is non-NULL, then the precomputed hashes are correct, which will no longer be necessarily true.

   In particular, if we wrote signing code that tried to use PrecomputedTransactionData to cache the hashes ahead of time for re-use, then I think this code would break (as the witnesses would all be NULL before the signatures are added, right?) So I think I’d prefer an approach that was easier to reason about…

5. Skip precompute sighash for transactions without witness 0da49b5926
6. jl2012 force-pushed on Jan 19, 2017
7. 
   jl2012 commented at 8:14 am on January 19, 2017: contributor

   @sdaftuar added a bool to indicate whether the cache is ready so it becomes fail-safe. If we want to use the cache in signing code (which is currently not), an argument could be added to PrecomputedTransactionData::PrecomputedTransactionData to request for that.
8. 
   jl2012 commented at 8:21 am on January 19, 2017: contributor

   And I believe it’s non-negligible. Assuming a block is full of non-witness txs (which all currently are), around 0.35MB of hashing is needed. For 100,000 blocks that’d be 35GB, which should take a few minutes at least.
9. 
   gmaxwell commented at 8:30 pm on January 19, 2017: contributor

   Concept ACK. Probably the most important performance measure is connectTip latency at runtime, and I think this could quite possibly dominate that figure.
10. 
    sipa commented at 4:07 pm on April 9, 2017: member

    utACK 0da49b5926b678b2ec35fabe37034f3d2e8385f4
11. 
    jl2012 commented at 2:35 pm on September 25, 2017: contributor

    @sdaftuar @gmaxwell does it look ok?
12. 
    sdaftuar commented at 1:43 pm on September 29, 2017: member

    utACK
13. 
    TheBlueMatt commented at 3:12 pm on September 29, 2017: member

    utACK 0da49b5926b678b2ec35fabe37034f3d2e8385f4 (in the future we really need to figure out a way to move this into our CTransactionRef stuff so that this gets calculated once upon mempool-add and then not recalculated when we get a block containing the same transaction).
14. 
    theuni commented at 8:39 pm on October 4, 2017: member

    I’d be a little more comfortable with this if PrecomputedTransactionData were passed around as const, so nobody’s tempted to mess with “ready” for some crazy reason. But that can always be done later.

    utACK 0da49b5926b678b2ec35fabe37034f3d2e8385f4

15. 
    laanwj commented at 5:49 pm on October 5, 2017: member

    Looks correct to me, utACK 0da49b5
16. laanwj merged this on Oct 5, 2017
17. laanwj closed this on Oct 5, 2017

    ---

18. laanwj referenced this in commit 17f2acedbe on Oct 5, 2017
19. 
    sipa commented at 5:52 pm on October 5, 2017: member

    utACK 0da49b5926b678b2ec35fabe37034f3d2e8385f4
20. DrahtBot locked this on Sep 8, 2021

Contributors
jl2012 sdaftuar gmaxwell sipa TheBlueMatt theuni laanwj

Labels
Refactoring