Cache witness hash in CTransaction #13011

maflcko · 2018-04-17T21:05:42Z

This speeds up:

compactblocks (v2)
ATMP
validation and miner (via BlockWitnessMerkleRoot)
sigcache (see also unrelated Faster sigcache nonce #13204)
rpc and rest (nice, but irrelevant)

This presumably slows down rescan, which uses a CTransaction and its GetHash, but never uses the GetWitnessHash. The slow down is proportional to the number of witness transactions in the rescan window. I.e. early in the chain there should be no measurable slow down. Later in the chain, there should be a slow down, but acceptable given the speedups in the modules mentioned above.

ryanofsky

utACK fae63d51ab252579adbaceb6aaba935bae75de88. Code looks ok, but what's the motivation for this change? I imagine there are a lot of performance implications.

Also, why is unique_ptr<uint256> used for the witness hash when the plain hash is just uint256?

ryanofsky · 2018-04-17T22:21:53Z

src/primitives/transaction.cpp

    }
-    return SerializeHash(*this, SER_GETHASH, 0);
+    return std::unique_ptr<const uint256>{new uint256{SerializeHash(*this, SER_GETHASH, 0)}};


Could use MakeUnique<const uint256>(SerializeHash(*this, SER_GETHASH, 0)); here

ryanofsky · 2018-04-17T22:25:39Z

src/primitives/transaction.cpp

-CTransaction::CTransaction(CMutableTransaction &&tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash(ComputeHash()) {}
+CTransaction::CTransaction() : vin(), vout(), nVersion(CTransaction::CURRENT_VERSION), nLockTime(0), hash{}, m_witness_hash{nullptr} {}
+CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}
+CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}


I believe you could remove some of the boilerplate and repetition here using member initialization in the header:

const std::unique_ptr<const uint256> m_witness_hash = ComputeWitnessHash();

It would be nice to do this for the other members, too.

I wonder what would be the execution order in this case? ComputeWitnessHash calls SerializeHash(*this, ...) so other members must be initialized before.

I wonder what would be the execution order in this case? ComputeWitnessHash calls SerializeHash(*this, ...) so other members must be initialized before.

Execution order always the matches order members are declared in the class, not the order initializations are written in the constructor (compilers will warn if the orders differ), so this wouldn't change behavior. You can see https://isocpp.org/wiki/faq/ctors#ctor-initializer-order for discussion about this.

We couldn't do the same for the hash member, I guess, so will leave this as is for now.

promag

Concept ACK.

I guess the extra memory usage is neglectable.

promag · 2018-04-18T07:03:17Z

src/primitives/transaction.h

-    // Compute a hash that includes both transaction and witness data
-    uint256 GetWitnessHash() const;
+    const uint256& GetHash() const { return hash; }
+    const uint256& GetWitnessHash() const { return m_witness_hash ? *m_witness_hash : hash; };


Could this expression be in ComputeWitnessHash?

practicalswift · 2018-04-18T13:03:27Z

Concept ACK

sdaftuar · 2018-04-19T12:45:45Z

@sipa raised design concerns with caching data like this in CTransaction previously (see #9700)

TheBlueMatt · 2018-04-20T16:01:28Z

Honestly I think we need to revive #9700. Now that SegWit has some use, giving up on the huge performance gains of that (and this) for (IMO) minor design concerns is an absurd tradeoff.

sipa · 2018-04-20T16:09:09Z

Then at the very least introduce alternate versions of CTransaction/CBlock that don't precompute anything, for use in reindexing and rescanning and serving blocks from disk. Otherwise you'll waste your time computing wtxids/sighashes in those cases, where they're never used.

TheBlueMatt · 2018-04-20T16:11:48Z

Indeed, though we may want to look into something wholesale smarter for rescan, its already impossibly slow even when considering the amount of I/O required.

Silently converting to a CMutableTransaction will drop all caches and should thus be done explicitly

maflcko · 2018-05-04T21:57:06Z

Split into two commits for easier review

TheBlueMatt · 2018-05-10T18:24:34Z

utACK fac1223. We should probably take this as-is IMO and then we can clean things up when we look at fully splitting out hashes in #13098.

jamesob · 2018-05-10T20:10:01Z

utACK fac1223

Numbers would be nice, but the code looks good.

ryanofsky · 2018-05-11T14:04:44Z

Was a discussion about this in IRC meeting yesterday: https://botbot.me/freenode/bitcoin-core-dev/msg/99929243/

Beginning of discussion before it goes on to talk about related PRs like #13098

[19:10:54] <MarcoFalke> Background: The witness hash is used for all loose transactions, so caching it would speed up validation (e.g. ATMP and compact block relay). Also, the witness hash is equal to the "normal hash" for transactions without a witness, so there is overhead for rescan/reindex is currently minimal (since there are not many transactions with witness). The gains of caching the witness hash dwarf any overhead during rescan/reindex, imo. And of course, we
[19:10:55] <MarcoFalke> can just rework rescan in a future pr.
[19:11:32] <MarcoFalke> The code changes are trivial, so at least that shouldn't be an issue
[19:12:03] <jamesob> would be nice if that paragraph was in the PR description
[19:12:23] <sipa> Downside is that it makes the transactions larger, and hardcodes some validation sprcific logic into the transaction data structure (which for example also affects serving blocks from disks etc)
[19:12:46] <BlueMatt> upside is we rectify a significant performance regression
[19:12:48] <sipa> So my view is that we should separate the transactions-for-validation tyoe and simple mutable transactions
[19:13:01] <BlueMatt> and obviously all pre-segwit-activation blocks will not be served any slower
[19:13:03] <MarcoFalke> sipa: I think those can easily be fixed in future prs (I have one open for the
blocks serving from disks, and wumpus as well)
[19:13:19] <sipa> MarcoFalke: i'm aware, not objecting to amything
[19:13:21] <BlueMatt> personally, I find it really unacceptable that we have this huge performance regression and arent taking it as something to be fixed asap
[19:13:33] <MarcoFalke> sipa: I know, just posting for reference
[19:13:34] <sipa> just pointing out that we don't want to do this work everywhere
[19:13:48] <sipa> so concept ack, if we commit to separating those types
[19:14:01] <wumpus> agree with sipa
[19:14:05] <BlueMatt> yes, imo we should merge the per as-is now, and then when we look towards MarcoFalke's next pr splitting out the types (which is gonna be a lot more work) we will probably pull out both hashes then anyway
[19:14:16] <sipa> there are other reasons for separating those types as well, btw
[19:14:22] * BlueMatt would kinda recommending backporting the pr as-is, though

Continues https://botbot.me/freenode/bitcoin-core-dev/msg/99929408/

maflcko · 2018-05-11T14:28:03Z

This speeds up:

compactblocks (v2)
ATMP
validation and miner (via BlockWitnessMerkleRoot)
sigcache (see also unrelated Faster sigcache nonce #13204)
rpc and rest (nice, but irrelevant)

This presumably slows down rescan, which uses a CTransaction and its GetHash, but never uses the GetWitnessHash. The slow down is proportional to the number of witness transactions in the rescan window. I.e. early in the chain there should be no measurable slow down. Later in the chain, there should be a slow down, but acceptable given the speedups in the modules mentioned above.

maflcko · 2018-05-11T14:56:34Z

Note: You can run benchmarks such as:

./src/bench/bench_bitcoin --filter=MempoolEviction  # ATMP (unchecked) with 100% segwit transactions
./src/bench/bench_bitcoin --filter=DeserializeBlockTest  # Used by rescan, so this should be slower, but none of the txs have witness, so there is no difference in runtime

jimpo · 2018-05-13T02:42:56Z

If performance is a concern in contexts where the precomputed value isn't needed, couldn't this just lazily compute and cache the witness hash? There'd probably have to be some concurrency handling, I suppose.

sipa · 2018-05-13T02:46:18Z

avoid "some concurrency handling" was the reason for making these fields const in the first place.

The alternative is either indirections (through atomic pointers) or mutexes all over the place.

The solution to not computing it in places where it isn't needed is not using a type that caches these values.

promag · 2018-05-13T07:07:00Z

utACK fac1223.

jimpo · 2018-05-17T18:40:28Z

utACK fac1223

ryanofsky

utACK fac1223. Only change since previous review is making m_witness_hash member const uint256 instead of std::unique_ptr<const uint256>, and adding the new cleanup commit: "Make CMutableTransaction constructor explicit …"

sipa · 2018-05-17T20:43:36Z

utACK fac1223

danielsam · 2018-05-21T18:47:30Z

utACK fac1223

laanwj · 2018-05-23T17:26:10Z

utACK fac1223

fac1223 Cache witness hash in CTransaction (MarcoFalke) faab55f Make CMutableTransaction constructor explicit (MarcoFalke) Pull request description: This speeds up: * compactblocks (v2) * ATMP * validation and miner (via `BlockWitnessMerkleRoot`) * sigcache (see also unrelated #13204) * rpc and rest (nice, but irrelevant) This presumably slows down rescan, which uses a `CTransaction` and its `GetHash`, but never uses the `GetWitnessHash`. The slow down is proportional to the number of witness transactions in the rescan window. I.e. early in the chain there should be no measurable slow down. Later in the chain, there should be a slow down, but acceptable given the speedups in the modules mentioned above. Tree-SHA512: 443e86acfcceb5af2163e68840c581d44159af3fd1fce266cab3504b29fcd74c50812b69a00d41582e7e1c5ea292f420ce5e892cdfab691da9c24ed1c44536c7

fac1223 Cache witness hash in CTransaction (MarcoFalke) faab55f Make CMutableTransaction constructor explicit (MarcoFalke) Pull request description: This speeds up: * compactblocks (v2) * ATMP * validation and miner (via `BlockWitnessMerkleRoot`) * sigcache (see also unrelated bitcoin#13204) * rpc and rest (nice, but irrelevant) This presumably slows down rescan, which uses a `CTransaction` and its `GetHash`, but never uses the `GetWitnessHash`. The slow down is proportional to the number of witness transactions in the rescan window. I.e. early in the chain there should be no measurable slow down. Later in the chain, there should be a slow down, but acceptable given the speedups in the modules mentioned above. Tree-SHA512: 443e86acfcceb5af2163e68840c581d44159af3fd1fce266cab3504b29fcd74c50812b69a00d41582e7e1c5ea292f420ce5e892cdfab691da9c24ed1c44536c7

ryanofsky reviewed Apr 17, 2018

View reviewed changes

promag reviewed Apr 18, 2018

View reviewed changes

maflcko force-pushed the Mf1804-cacheWitnessHash branch from fae63d5 to fa77996 Compare April 21, 2018 13:37

laanwj added the Validation label Apr 23, 2018

maflcko mentioned this pull request Apr 27, 2018

Skip tx-rehashing on historic blocks #13098

Closed

TheBlueMatt added a commit to bitcoinfibre/bitcoinfibre that referenced this pull request May 4, 2018

empty_commit: Pull bitcoin#13011 follows

b15d030

MarcoFalke added 2 commits May 4, 2018 17:40

Make CMutableTransaction constructor explicit

faab55f

Silently converting to a CMutableTransaction will drop all caches and should thus be done explicitly

Cache witness hash in CTransaction

fac1223

maflcko force-pushed the Mf1804-cacheWitnessHash branch from fa77996 to fac1223 Compare May 4, 2018 21:55

maflcko mentioned this pull request May 13, 2018

bench: Add block assemble benchmark #13219

Merged

ryanofsky reviewed May 17, 2018

View reviewed changes

maflcko assigned laanwj May 17, 2018

laanwj merged commit fac1223 into bitcoin:master May 23, 2018

maflcko deleted the Mf1804-cacheWitnessHash branch May 23, 2018 17:50

Bushstar mentioned this pull request May 26, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#336

Merged

TheBlueMatt added a commit to bitcoinfibre/bitcoinfibre that referenced this pull request Jul 17, 2018

empty_commit: Pull bitcoin#13011 follows

cee355b

TheBlueMatt added a commit to bitcoinfibre/bitcoinfibre that referenced this pull request Sep 27, 2018

empty_commit: Pull bitcoin#13011 follows

5b62ef3

TheBlueMatt added a commit to bitcoinfibre/bitcoinfibre that referenced this pull request Dec 28, 2018

empty_commit: Pull bitcoin#13011 follows

8e09c07

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Cache witness hash in CTransaction #13011

Cache witness hash in CTransaction #13011

Uh oh!

Conversation

maflcko commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 17, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 17, 2018

Choose a reason for hiding this comment

Uh oh!

promag Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 17, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko May 17, 2018

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Apr 18, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Apr 18, 2018

Uh oh!

sdaftuar commented Apr 19, 2018

Uh oh!

TheBlueMatt commented Apr 20, 2018

Uh oh!

sipa commented Apr 20, 2018

Uh oh!

TheBlueMatt commented Apr 20, 2018

Uh oh!

maflcko commented May 4, 2018

Uh oh!

TheBlueMatt commented May 10, 2018

Uh oh!

jamesob commented May 10, 2018

Uh oh!

ryanofsky commented May 11, 2018

Uh oh!

maflcko commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 11, 2018

Uh oh!

jimpo commented May 13, 2018

Uh oh!

sipa commented May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented May 13, 2018

Uh oh!

jimpo commented May 17, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented May 17, 2018

Uh oh!

danielsam commented May 21, 2018

Uh oh!

laanwj commented May 23, 2018

Uh oh!

Uh oh!

maflcko commented Apr 17, 2018 •

edited

Loading

maflcko commented May 11, 2018 •

edited

Loading

sipa commented May 13, 2018 •

edited

Loading