Cache witness hash in CTransaction

MarcoFalke commented at 9:05 pm on April 17, 2018: member

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.

in src/primitives/transaction.cpp:75 in fae63d51ab outdated

73     if (!HasWitness()) {
74-        return GetHash();
75+        return nullptr;
76     }
77-    return SerializeHash(*this, SER_GETHASH, 0);
78+    return std::unique_ptr<const uint256>{new uint256{SerializeHash(*this, SER_GETHASH, 0)}};

ryanofsky commented at 10:21 pm on April 17, 2018:

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

in src/primitives/transaction.cpp:81 in fae63d51ab outdated

82-CTransaction::CTransaction() : vin(), vout(), nVersion(CTransaction::CURRENT_VERSION), nLockTime(0), hash() {}
83-CTransaction::CTransaction(const CMutableTransaction &tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash(ComputeHash()) {}
84-CTransaction::CTransaction(CMutableTransaction &&tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash(ComputeHash()) {}
85+CTransaction::CTransaction() : vin(), vout(), nVersion(CTransaction::CURRENT_VERSION), nLockTime(0), hash{}, m_witness_hash{nullptr} {}
86+CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}
87+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()} {}

ryanofsky commented at 10:25 pm on April 17, 2018:

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

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

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

promag commented at 7:01 am on April 18, 2018:

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

ryanofsky commented at 8:17 pm on May 17, 2018:

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.

MarcoFalke commented at 8:30 pm on May 17, 2018:

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

ryanofsky commented at 10:29 pm on April 17, 2018: member

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?

in src/primitives/transaction.h:317 in fae63d51ab outdated

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

promag commented at 7:03 am on April 18, 2018:

Could this expression be in ComputeWitnessHash?

promag commented at 7:05 am on April 18, 2018: member

Concept ACK.

I guess the extra memory usage is neglectable.

practicalswift commented at 1:03 pm on April 18, 2018: contributor

Concept ACK

sdaftuar commented at 12:45 pm on April 19, 2018: member

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

TheBlueMatt commented at 4:01 pm on April 20, 2018: member

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 commented at 4:09 pm on April 20, 2018: member

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 commented at 4:11 pm on April 20, 2018: member

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.

MarcoFalke force-pushed on Apr 21, 2018

laanwj added the label Validation on Apr 23, 2018

TheBlueMatt referenced this in commit b15d0303a9 on May 4, 2018

Make CMutableTransaction constructor explicit

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

faab55fbb1

Cache witness hash in CTransaction fac1223a56

MarcoFalke force-pushed on May 4, 2018

MarcoFalke commented at 9:57 pm on May 4, 2018: member

Split into two commits for easier review

TheBlueMatt commented at 6:24 pm on May 10, 2018: member

utACK fac1223a568fa1ad6dd602350598eed278d115e8. 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.

laanwj added this to the "Blockers" column in a project

jamesob commented at 8:10 pm on May 10, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/13011/commits/fac1223a568fa1ad6dd602350598eed278d115e8

Numbers would be nice, but the code looks good.

ryanofsky commented at 2:04 pm on May 11, 2018: member

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/

MarcoFalke commented at 2:28 pm on May 11, 2018: member

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.

MarcoFalke commented at 2:56 pm on May 11, 2018: member

Note: You can run benchmarks such as:

0./src/bench/bench_bitcoin --filter=MempoolEviction  # ATMP (unchecked) with 100% segwit transactions
1./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 commented at 2:42 am on May 13, 2018: contributor

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 commented at 2:46 am on May 13, 2018: member

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 commented at 7:07 am on May 13, 2018: member

utACK fac1223.

jimpo commented at 6:40 pm on May 17, 2018: contributor

utACK fac1223

ryanofsky commented at 8:12 pm on May 17, 2018: member

utACK fac1223a568fa1ad6dd602350598eed278d115e8. 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 commented at 8:43 pm on May 17, 2018: member

utACK fac1223a568fa1ad6dd602350598eed278d115e8

MarcoFalke assigned laanwj on May 17, 2018

danielsam commented at 6:47 pm on May 21, 2018: none

utACK fac1223

laanwj commented at 5:26 pm on May 23, 2018: member

utACK fac1223a568fa1ad6dd602350598eed278d115e

laanwj merged this on May 23, 2018

laanwj closed this on May 23, 2018

laanwj referenced this in commit 3c2a41a9fc on May 23, 2018

MarcoFalke deleted the branch on May 23, 2018

fanquake removed this from the "Blockers" column in a project

TheBlueMatt referenced this in commit cee355bb99 on Jul 17, 2018

TheBlueMatt referenced this in commit 5b62ef3a3a on Sep 27, 2018

TheBlueMatt referenced this in commit 8e09c0778b on Dec 28, 2018

UdjinM6 referenced this in commit c85dc3df80 on Jun 18, 2021

UdjinM6 referenced this in commit 26087f4dd1 on Jun 24, 2021

UdjinM6 referenced this in commit f33ed5594a on Jun 26, 2021

UdjinM6 referenced this in commit 408d0a2afc on Jun 26, 2021

UdjinM6 referenced this in commit c8ee5e1665 on Jun 28, 2021

MarcoFalke locked this on Sep 8, 2021

Cache witness hash in CTransaction #13011