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-
jl2012 commented at 7:55 am on January 18, 2017: contributorThis saves unnecessary hash caching for non-segwit transactions, but I am not sure if the difference is noticeable.
-
jl2012 force-pushed on Jan 18, 2017
-
jonasschnelli added the label Refactoring on Jan 18, 2017
-
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…
-
Skip precompute sighash for transactions without witness 0da49b5926
-
jl2012 force-pushed on Jan 19, 2017
-
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.
-
jl2012 commented at 8:21 am on January 19, 2017: contributorAnd 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.
-
gmaxwell commented at 8:30 pm on January 19, 2017: contributorConcept ACK. Probably the most important performance measure is connectTip latency at runtime, and I think this could quite possibly dominate that figure.
-
sipa commented at 4:07 pm on April 9, 2017: memberutACK 0da49b5926b678b2ec35fabe37034f3d2e8385f4
-
sdaftuar commented at 1:43 pm on September 29, 2017: memberutACK
-
TheBlueMatt commented at 3:12 pm on September 29, 2017: memberutACK 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).
-
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
-
laanwj commented at 5:49 pm on October 5, 2017: memberLooks correct to me, utACK 0da49b5
-
laanwj merged this on Oct 5, 2017
-
laanwj closed this on Oct 5, 2017
-
laanwj referenced this in commit 17f2acedbe on Oct 5, 2017
-
sipa commented at 5:52 pm on October 5, 2017: memberutACK 0da49b5926b678b2ec35fabe37034f3d2e8385f4
-
DrahtBot locked this on Sep 8, 2021
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
More mirrored repositories can be found on mirror.b10c.me