Skip witness sighash cache for non-segwit transactions #9572

pull jl2012 wants to merge 1 commits into bitcoin:master from jl2012:nocache changing 2 files +12 −6
  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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me