UTXO hasher abstraction #65
Conversation
domob1812
force-pushed
the
utxo-hasher
branch
from
7e2d921 to
63d3a81
Compare
domob1812
force-pushed
the
utxo-hasher
branch
from
63d3a81 to
450946f
Compare
domob1812
force-pushed
the
utxo-hasher
branch
from
450946f to
a8c43d3
Compare
domob1812
force-pushed
the
utxo-hasher
branch
4 times, most recently
from
c60b17f to
b0939c1
Compare
domob1812
force-pushed
the
utxo-hasher
branch
5 times, most recently
from
c37d3dd to
2be9b53
Compare
|
Marked as draft for now. It is ready for review, but #73 should be merged first (that's the first commit of this one). |
There was a problem hiding this comment.
From what I've seen so far, one main thing seems to be missing. Mainly that the CCoinsViewCache instances (and presumably other parent classes in that hierarchy) should perhaps depend on the utxo hasher being used (perhaps as part of instantiation?). The reason is that they keep track of the utxo set, by hash. So any changes to how the utxo set is referenced would impact that.
| @@ -203,7 +203,7 @@ std::vector<TxPriority> BlockMemoryPoolTransactionCollector::PrioritizeMempoolTr | |||
| // Read prev transaction | |||
| if (!view.HaveCoins(txin.prevout.hash)) { | |||
There was a problem hiding this comment.
The view, as its looking for coins by hash should perhaps also depend on the utxo hasher in some fashion? Not saying this is the best idea, just pointing out that it doesn't seem like this behaviour is covered at all by the current changes.
There was a problem hiding this comment.
The hasher actually comes into play not for referencing the UTXO set, but for creating the UTXOs from transactions. So in my opinion, that is something that logically happens before CCoinsViewCache is even involved; the CCoinsView then just stores the UTXO-hash-keyed entries, and allows looking them up. Hence I believe the UTXO hasher should not be connected to the coins view.
| uint256 BlockUtxoHasher::GetUtxoHash(const CTransaction& tx) const | ||
| { | ||
| return tx.GetHash(); | ||
| } |
There was a problem hiding this comment.
Why inherit again from the TransactionUtxoHasher when the class itself already provides the necessary functionality? Seems like that would add indirection
There was a problem hiding this comment.
A previous version of this PR was like that. But then I decided to split the hasher. Now TransactionUtxoHasher is actually an abstract class, and the hashing function is not implemented at all.
BlockUtxoHasher is where we implement the specific hashing behaviour we need for processing a block; MemoryUtxoHasher and (in a later PR) WalletUtxoHasher are alternative implementations with different behaviour.
Thus I think it makes sense to keep the interface / superclass and the implementations separated.
domob1812
force-pushed
the
utxo-hasher
branch
3 times, most recently
from
ad8abd5 to
c2c29f5
Compare
|
I've now also updated the BIP30 checking code to use the UTXO hasher as well. This has no effect in practice, since it only applies to PoW coinbase transactions, and a) those have the same bare txid as their normal hash since #73, and b) none of them will be created after segwit light activation. But it is still the correct thing to do and makes the code clear. |
domob1812
force-pushed
the
utxo-hasher
branch
2 times, most recently
from
6d4d0d1 to
f53dfc0
Compare
Wrap some of the functions used only in main into an anonymous namespace; some of them were already marked as "static", others were not (but they were still not needed from anywhere else). This makes it immediately clear that those functions are only used in the context of main. FindUndoPos had a declaration first and the definition later, but it is fine to just move the definition to where the declaration was. This simplifies the code further (and is a pure move in the context of this commit).
This is a bunch of related and straight-forward cleanups to the code around masternode configuration: Pass arguments as const& instead of by value, mark functions as const, and move helper functions from being static in a class and declared inside the header to just being inside an anonymous namespace in the cpp file.
Some places in the code need to determine the hash by which the UTXOs of a given transaction will be referred to; in particular, we need that when processing UTXO set updates for block connects and disconnects, in the mempool and for assembling transactions into a new block. This commit introduces a class TransactionUtxoHasher, which abstracts this step and is used in all those places in the code instead of just getting the txid directly. For now, this has no effects on behaviour; but it makes it more clear in the code where we need this particular logical feature; it will allow us to add some more unit tests for those parts with explicit mocks of the hasher class; and it will make it easier to implement segwit-light in the future (where we basically just need to flip the hasher implementation but no other parts in the code).
Add a unit test (together with the necessary framework around) for UTXO creation from UpdateCoins, based on the UTXO hasher (rather than the txid directly).
This extends the mempool unit tests to explicitly verify that adding transactions, removing transactions, checking the pool and looking up coins / transactions still works even if we use the bare txid for some transactions as UTXO hash (as will be the case with segwit-light in the future).
|
Reopened as #80 with updated branches. |
This creates a new
TransactionUtxoHasherabstraction: The class is responsible for mapping a givenCTransactionto theuint256that is used to refer to the UTXOs created by the transaction. This is currently just the normal txid (tx.GetHash()), but in the future with segwit-light (#61) will be changed.For now, this PR just refactors the code to make it explicit where in the consensus and mempool (not yet the wallet) we actually use this concept of "UTXOs of a given transaction". Apart from unit tests, the hasher always returns the txid that was used inline before, so this does not affect consensus or introduces a fork yet. But it will make it easier to do so in the future.