[pull] master from bitcoin:master #1451
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The chainstate UTXO database only stores unspent outputs; spent entries are removed. Assert after reading a `Coin` so corruption or misuse cannot propagate a spent coin through the `GetCoin()` interface.
Production `GetCoin()` implementations only return unspent coins. Update the `CCoinsView` test backend to match that contract, so tests stop exercising cache states that cannot occur with `CCoinsViewCache` or `CCoinsViewDB`. Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
`CoinsViewBottom` roughly simulates a memory-backed `CCoinsViewDB`, which never stores spent coins. Stop returning spent coins from `GetCoin()`, erase spent entries in `BatchWrite()`, and tighten comparisons to expect `std::nullopt` when the simulator has no coin. Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
`CCoinsViewCache::FetchCoin()` had special handling for a spent `Coin` returned by the parent view. Production parents (`CCoinsViewCache` and `CCoinsViewDB`) do not return spent coins, so this path is unreachable. Replace it with an `Assume(!coin.IsSpent())`, drop outdated documentation about spent+FRESH cache entries, and simplify `SanityCheck()` to assert the remaining possible state invariants. This is safe because it does not change behavior for valid backends and will fail fast if the `GetCoin()` contract is violated. Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
removeprunedfunds removes all entries from mapTxSpends for the inputs of the pruned tx. However, this is incorrect, because there could be multiple entries from conflicting transactions (that shouldn't be removed as well). This could lead to the wallet creating invalid transactions, trying to double spend utxos. The bug persists when the conflicting tx was mined, because the wallet trusts its internal accounting instead of calling AddToSpends again.
Verify that addresses announcing themselves (addr == source) are exempt from time penalties, while addresses announced by others receive the expected penalty.
…sactions 1f60ca3 wallet: fix removeprunedfunds bug with conflicting transactions (Martin Zumsande) Pull request description: `removeprunedfunds` removes all entries from `mapTxSpends` for the inputs of the pruned tx. However, this is incorrect, because there could be multiple entries from conflicting transactions (that shouldn't be removed as well). This could lead to the wallet creating invalid transactions, trying to double spend utxos. The bug persists when the conflicting tx was mined, because the wallet trusts its internal accounting instead of calling `AddToSpends` again. The added test should fail on master. ACKs for top commit: achow101: ACK 1f60ca3 fjahr: tACK 1f60ca3 furszy: utACK 1f60ca3 vasild: ACK 1f60ca3 Tree-SHA512: 3cc9ed547530fd53e25721177b76ab2e1eae16ce2c0e63fc01b20fdbf8bd02655dae51167ad56f9dec748d34c61ce65d38f993370820601f8257c73b876a3347
…t coins 2ee7f9b coins: assume `GetCoin` only returns unspent coins (Andrew Toth) eec551a fuzz: keep `coinscache_sim` backend free of spent coins (Andrew Toth) 3e4155f test: do not return spent coins from `CCoinsViewTest::GetCoin` (Andrew Toth) ee1e40f txdb: assert `CCoinsViewDB::GetCoin` only returns unspent coins (Lőrinc) Pull request description: This PR is split out from #33018 to keep that PR focused on removing the `FRESH-but-not-DIRTY` cache state. ### Problem `::GetCoin()` is an interface for querying the UTXO set, so production implementations should only ever return unspent coins. Tests should mimic this to provide useful feedback. ### Fix: * Add a fail-fast assertion that `CCoinsViewDB::GetCoin()` never returns a spent coin. * Align unit tests and fuzz simulations with the production `GetCoin()` contract by never returning spent coins. * Replace the unreachable “spent coin returned by parent” handling in `CCoinsViewCache::FetchCoin()` with `Assert(!coin.IsSpent())`, drop outdated `spent+FRESH` docs, and tighten `SanityCheck()` invariants. Behavior is unchanged, it just aligns our tests to exercise valid states. ACKs for top commit: andrewtoth: re-ACK 2ee7f9b optout21: crACK 2ee7f9b achow101: ACK 2ee7f9b w0xlt: reACK 2ee7f9b Tree-SHA512: be21cc09690410fc04ca25e1ba47aae6186bc037e413b3bb1e6e9a04e6364cbfac5a2fcdc49b638fec848cd29243fab0cc0581b9923f34fafe8366828f690ed4
…ling e770392 test: addrman: test self-announcement time penalty handling (Bruno Garcia) Pull request description: This PR adds a test case for addrman that verifies that addresses announcing themselves (addr == source) are exempt from time penalties, while addresses announced by others receive the expected penalty. It fixes the following mutant (https://corecheck.dev/mutation/src/addrman.cpp#L561): ```diff diff --git a/src/addrman.cpp b/src/addrman.cpp index 206b541..c6a045fd8d 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -558,7 +558,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, std::c AddrInfo* pinfo = Find(addr, &nId); // Do not set a penalty for a source's self-announcement - if (addr == source) { + if (addr != source) { time_penalty = 0s; } ``` ACKs for top commit: maflcko: review ACK e770392 🐤 achow101: ACK e770392 fjahr: Code review ACK e770392 naiyoma: tACK e770392 Tree-SHA512: ec029d1e1e979f91840af944984cad530a1ce9a0eceb123230817f0ef3b9ad47253eebc4c953d350de2d904b59496fcd4757123c8bd63cf0e09c3581da48fff8
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )