Cleanups related to the OutputHash work #95
Conversation
domob1812
force-pushed
the
output-hash-cleanups
branch
5 times, most recently
from
ec47ce0 to
21f10f7
Compare
This adds CCoins::ToString as a helper method that prints a representation of a CCoins instance. The method is not called for now in the code, but can be useful both for future logging and also just for ad-hoc log statements when debugging CCoins-related code.
Introduce and use a type alias for the std::map type that holds the data for mapping outputs to depending orphan transactions in BlockMemoryPoolTransactionCollector. This is quite a length type when spelled out, and used in a couple of functions as argument. Using a type alias makes it more readable and also easier to adjust in the future as needed.
Logging.cpp defined a "private" version of the begin_ptr and end_ptr templates that are also defined in serialize.h. This replaces that with simply using serialize.h. In addition to avoiding code duplication, this also makes sure that the code still works even if Logging.cpp were ever to include directly or indirectly serialize.h (where without this commit, it would lead to an ambiguous template call).
TransactionsByHash is defined in VaultManager.h, but actually never used in the code at all.
Instead of spelling out iterator types, use auto.
Pass some arguments as const& instead of by value, and mark some variables as const where it is possible. Also includes a trivial change to serialisation, where we explicitly list the two fields in COutPoint instead of serialising as flat data.
Instead of constructing an object and inserting it, use emplace_back for adding to transaction inputs.
In CCoinsViewMemPool, mark the overriding methods actually as override. This ensures that the compiler will enforce and catch any potential changes to the superclass method signature.
Fix a mistyped variable name. Also use the previous transaction hash directly when emitting a notification, instead of relying on the prevout value of the spending transaction. This is more explicit and decouples the meaning (e.g. for segwit light in the future).
rpcrawtransaction.cpp contains two places that use heuristic transaction lookups by txid: One in the spent index to see if there are transations spending its outputs, and one in sendrawtransaction to check the UTXO set as a cheap heuristic on whether or not the transaction is already contained in the blockchain. This extends both places to try looking up both the txid and also the bare txid. At most one of them can ever match (unless there is a collision in SHA-256), so it doesn't hurt, but it will ensure that the code works independent of the state of segwit light.
domob1812
force-pushed
the
output-hash-cleanups
branch
from
21f10f7 to
022d653
Compare
|
This one looks good just except for the iterator types not being spelled out. I don't trust the compiler to do anything "non-creative" with auto in most cases. Moreover since the changes are merely regarding single use iterators in each case, it doesn't really add any benefit that is readily evident. What's your take on this? |
Indeed, that is a matter of taste. However, I personally think that usage with iterators is more or less the canonical use of In my opinion, it makes the code a lot simpler to read, and in particular also means that we need to change fewer places when we change The alternative (a bit less trust in the compiler, a bit more complex code still, but also no need to update all iterations if you update the type of a container) would be using something like |
This is a collection of straight-forward cleanups and refactorings, which have been split out of #80 but are independent of the UTXO hasher or segwit light.