CoinsByPrevOuts to TxIdsByPrevOuts #11790
Conversation
| @@ -118,16 +118,17 @@ private bool TryAddNoLock(SmartCoin coin) | |||
| { | |||
| hashSet = new(); | |||
| CoinsByTransactionId.Add(coin.TransactionId, hashSet); | |||
|
|
|||
| // Each prevOut of the transaction contributes to the existence of coins. | |||
| // This only has to be added once per transaction. | |||
There was a problem hiding this comment.
What is the reasoning for that?
There was a problem hiding this comment.
It's immediately clear what behavior TryAddNoLock should have for double spends.
My approach would be to throw an UnreachableException whenever CoinsRegistry is used incorrectly. So adding another coin that spends already spent transaction output would lead to an exception being thrown. The reason being that caller failed to call the CoinsRegistry.Undo method.
It follows that
TxIdsByPrevOuts[input.PrevOut] = coin.TransactionId;is forgiving implementation, while
if (!TxIdsByPrevOuts.TryAdd(input.PrevOut, coin.TransactionId)) throw ...would be unforgiving (imho better).
I don't have time to check it more in detail right now but documenting this would be really valuable.
There was a problem hiding this comment.
Here the reason is that we will call this loop once per output, but each output references the same object SmartTransaction, so its inputs will be the same at each iteration.
I agree that it would be better to throw here. The reason I'm not doing that is to be safe and to follow the legacy behaviour of this cache, as it was already working like that prior to our recent changes to the CoinsRegistry. I have to think a bit more about the implications of throwing here. Also, why throw and don't call Undo?
There was a problem hiding this comment.
Also, why
throwand don't callUndo?
It would throw only if there is a serious bug in code somewhere. So one should definitely call Undo. However, if the cache gets corrupted, we should throw (in general). We don't and bugs get swallowed. That's what I meant really.
There was a problem hiding this comment.
2bc1885 I had to fix some tests that were creating txs with several times the same OutPoint. I think it was proof that throwing here is indeed a good idea.
There was a problem hiding this comment.
Well-spotted! Good work.
turbolay
force-pushed
the
CoinsByPrevOutsToTxId
branch
from
94d56ea
to
7c160fd
Compare
# Conflicts: # WalletWasabi/Blockchain/TransactionOutputs/CoinsRegistry.cs
|
Sorry for the other problems with case, merging accident I believe! |
|
@turbolay Please check by changes if you agree with them. If the tests are green (locally they are for me), then I'll approve and we can merge. |
Next 2 commits from #11740
c0ef1e9
5fe794a
It makes a lot of sense IMO because the values in this cache are already grouped by TxId