Safer modify new coins #9107
Safer modify new coins #9107
Conversation
|
Concept ACK, thanks for improving documentation around this code. |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Tested ACK 3572fea7dce3c20bfca237a53353cdf97c0f3669.
src/test/coins_tests.cpp
Outdated
There was a problem hiding this comment.
nit: "ids" instead of "txs" would be more consistent with other names
src/test/coins_tests.cpp
Outdated
There was a problem hiding this comment.
Arg could be const reference
src/test/coins_tests.cpp
Outdated
There was a problem hiding this comment.
Is this special case actually needed? Seems like origcoins/oldcoins will be empty in this case anyway.
There was a problem hiding this comment.
It seems cleaner to me to not be messing with tx.vin of coinbases
src/test/coins_tests.cpp
Outdated
There was a problem hiding this comment.
Suggestion: Maybe move the utxoset updates here and elsewhere next to the result updates, since utxoset is essentially just a list of the keys present in the result map. If somebody is changing one of these, they probably need to update the other too.
There was a problem hiding this comment.
hmm.. I'll move this above the alltxs insert, but I was trying to keep the order the same for each set of changes.
- update expected result
- modify cache the same way the production code does
(these first two things are what we will compare against each other) - modify state used to run the test (which includes utxoset)
|
utACK 743009682111aef5c4aab84c0cb5189e4afa4f06 |
morcos
force-pushed
the
saferModifyNewCoins
branch
from
7430096 to
c9270c9
Compare
|
Squashed |
src/coins.cpp
Outdated
There was a problem hiding this comment.
I hate bip numbers...any chance you can also/or only state what this actually means?
There was a problem hiding this comment.
(I think you did for 30, but not for 34)
|
While you're at it, can you add a comment to coins.cpp:200 (last else branch in BatchWrite) noting that it is very important that we are only swapping the coins itself, and keeping the flags set on the parent cache. |
|
utACK code changes in c9270c96e2ff1676eb87a94ef9b872bea3653804, though I'd prefer more comments. Didnt review tests, but did spend a bunch of time looking at this with @morcos when he was deciding the best route to go for changes. |
|
@TheBlueMatt tell me if this is what you had in mind? I'm not sure it's worth it, but don't mind squashing the additional comments if you want them. The whole design of coins would be broken if we were copying all the flags over, but I tried to flag the confusing case. |
|
Yea, looks good to me. |
morcos
force-pushed
the
saferModifyNewCoins
branch
from
f166b1f to
8174c1d
Compare
|
squashed in additional comments |
src/coins.cpp
Outdated
There was a problem hiding this comment.
Just a suggestion, but I think another way you could write this section might be:
if (!coinbase) {
// New coins must not already exist.
assert(ret.first->second.coins.IsPruned());
if (!(ret.first->second.flags & CCoinsCacheEntry::DIRTY)) {
// If the coin is known to be pruned (have no unspent outputs) in
// the current view and the cache entry is not dirty, we know the
// coin also must be pruned in the base view as well, so it is safe
// to mark this fresh.
ret.first->second.flags |= CCoinsCacheEntry::FRESH;
}
}
Differences from above:
- Assert is broader and more prominent.
- Comments are inline, which I think make this more understandable.
- FRESH flag will continue to be set in the case of the non-dirty existing cache entry.
There was a problem hiding this comment.
heh. i'll want to think about that 100 times, but that sounds right to me
morcos
force-pushed
the
saferModifyNewCoins
branch
from
8174c1d to
bd5d189
Compare
|
dancing the dance of a clean rebase on the grave of main.cpp will address @ryanofsky's suggestion separately |
|
@sipa @TheBlueMatt Can you guys take a look one more time at the new logic. I'll squash to one commit after you ACK. @fanquake Can you milestone for 0.14.0 |
sipa
left a comment
sipa
left a comment
There was a problem hiding this comment.
utACK e5f93783cd4c55a4a040719a9006e897e44509a3
src/coins.cpp
Outdated
There was a problem hiding this comment.
This line could move into an else branch.
There was a problem hiding this comment.
Which line? The Clear()? Isn't that needed for the historical coinbase overwrite cases?
|
The branch above starts with an assert that coins.IsPruned(), so a Clear()
in that case would be a no-op (I think?).
|
|
oh yes, confused myself.. |
|
Test fixes are here: https://github.com/ryanofsky/bitcoin/commits/pr/morcos-saferModifyNewCoins (in the two squash commits) if you see failures after rebasing. |
|
@morcos No strong opinion, just pointing it out. I don't think it will have a noticable performance impact. |
We were marking coins FRESH before being sure they were not overwriting dirty undo data. This condition was never reached in existing code because undo data was always flushed before UpdateCoins was called with new transactions, but could have been exposed in an otherwise safe refactor. Clarify in the comments the assumptions made in ModifyNewCoins. Add ability to undo transactions to UpdateCoins unit test. Thanks to Russ Yanofsky for suggestion on how to make logic clearer and fixing up the ccoins_modify_new test cases.
morcos
force-pushed
the
saferModifyNewCoins
branch
from
e5f9378 to
b50cd7a
Compare
|
Rebased and squashed Ignored @sipa's suggestion (sorry) |
|
utACK b50cd7a |
b50cd7a Fix dangerous condition in ModifyNewCoins. (Alex Morcos)
|
I ran the simulation test 1000x longer than the normal unit tests do. |
b50cd7a Fix dangerous condition in ModifyNewCoins. (Alex Morcos)
cdbb1a6 Remove unused variable in test, fixing warning. (Russell Yanofsky) 03031a5 Check FRESH validity in CCoinsViewCache::BatchWrite (Russell Yanofsky) 3fb3e03 Fix dangerous condition in ModifyNewCoins. (random-zebra) 228d6bc [test] Add CCoinsViewCache Access/Modify/Write tests (random-zebra) Pull request description: Going on with the coins view cache update work (started with #1774 and #1777). This backports the following upstream PRs (adapting them with the assumption that duplicate coinbase transactions are not possible): - bitcoin#9308 - [test] Add CCoinsViewCache Access/Modify/Write tests - bitcoin#9107 - Safer modify new coins - bitcoin#9310 - Assert FRESH validity in CCoinsViewCache::BatchWrite - bitcoin#9435 - Removed unused variable in test, fixing warning. Note: Will rebase #1773 on top of this one (to change `ApplyTxInUndo` return value in the unit test too). ACKs for top commit: furszy: pretty nice one, ACK cdbb1a6 Fuzzbawls: ACK cdbb1a6 Tree-SHA512: 69c534da1083c3c4a535923e98da7750474c6698bb45e042778b539a27fab0455d78aaf670bed9485522f9d2ee25e4ad57fa7d35632523fe7376fe5f5febb1e5
6 participants
This makes ModifyNewCoins more conservative in when it marks newly created outputs as FRESH. The usage of ModifyNewCoins in the code base doesn't actually require this because its only ever used on a new cache, however I think its dangerous code that is just asking for a consensus mistake to be used in the future. The comments in the code hopefully clarify a bit better the proper assumptions that need to be made going forward.
EDIT: The changes to the unit test are a bit complicated, but they demonstrate the problem. They fail without the prior commit.