1c36bafc5f wallet: have prune error take precedence over assumedvalid (James O'Beirne)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/pull/23997#discussion_r891412739.
From Russ Yanofsky:
> Agree with all of Marco's points here and think this should be updated
>
> If havePrune and hasAssumedValidChain are both true, better to show havePrune error message. Assumed-valid error message is vague and not very actionable. Would suggest "Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}"
ACKs for top commit:
MarcoFalke:
ACK 1c36bafc5f
aureleoules:
ACK 1c36bafc5f
Tree-SHA512: bfb0024bb962525cbbd392ade3c0331a8b0525e7f2f2ab52b2dbb9b6dd6311070d85ecb762a7689db84a30991971865698ab6fec187206e6a92133790c5a91dc
faa15527d7 test: Use dedicated mempool in TestBasicMining (MacroFake)
fafab384a0 test: Use dedicated mempool in TestPackageSelection (MacroFake)
fa4055d79c test: Use dedicated mempool in TestPrioritisedMining (MacroFake)
fa29218285 test: Pass mempool reference to AssemblerForTest (MacroFake)
Pull request description:
This cleans up the miner tests:
* Removes duplicate/redundant and thus confusing chainparams object.
* Uses a fresh mempool for each subtest instead of using the "global" one from the testing setup. This makes it easier to follow the tests in smaller scopes. Also it makes sure the mempool is truly cleared by reconstructing it. Finally, this removes calls to `clear`, see https://github.com/bitcoin/bitcoin/pull/19909
ACKs for top commit:
glozow:
utACK faa15527d7
Tree-SHA512: ced1260f6ab70fba74b0fac7ff4fc7adfddcd2f3bee785249d2a4a9055ac253eff9090edbda7a17e72a71a81b56ff708d5ff64e1f57ebc7b7747d6c88fec51e3
adb1714426 Fix comment typos in scriptpubkeyman.cpp, wallet.cpp, wallet.h (Dimitris Tsapakidis)
Pull request description:
Fixes a number of comment typos found in the code.
Top commit has no ACKs.
Tree-SHA512: c2c996b66d33ecf0ee734b76303a0f2444e184d2f3ff6931768712ca51011ad51e54336c33a2ff55133766d20ae6adcbb14ddc754dde58b1fe9167d68f54fec5
Use `UniqueLock` directly. Type deduction works just fine from the first
argument to the constructor of `UniqueLock`, so there is no need to
repeat
```cpp
UniqueLock<typename std::remove_reference<typename std::remove_pointer<decltype(cs)>::type>::type>
```
five times in the `LOCK` macros. Just `UniqueLock` suffices.
Use `MutexType` instead of `Mutex` for the template parameter of
`UniqueLock` because there is already a class named `Mutex` and the
naming overlap is confusing. `MutexType` is used elsewhere in `sync.h`.
8891949bdc index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Ryan Ofsky)
Pull request description:
Since commit f08c9fb0c6 from PR https://github.com/bitcoin/bitcoin/pull/21726, index `BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has also been a race condition in the `coinstatsindex_initial_sync` unit test.
It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable.
Also since commit f08c9fb0c6, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index. But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in https://github.com/bitcoin/bitcoin/issues/25365#issuecomment-1259744133
This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, https://github.com/bitcoin/bitcoin/pull/26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors https://github.com/bitcoin/bitcoin/issues/25365.
There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down.
Co-authored-by: vasild
Co-authored-by: MarcoFalke
ACKs for top commit:
mzumsande:
re-ACK 8891949bdc
Tree-SHA512: 52e29e3772a0c92873c54e5ffb31dd66a909b68a2031b7585713cd1d976811289c98bd9bb41679a8689062f03be4f97bb8368696e789caa4607c2fd8b1fe289b
fabbbe32ee Remove unused CDataStream::rdbuf method (MacroFake)
Pull request description:
It is unused and seems unlikely to be ever used.
ACKs for top commit:
theStack:
Code-review ACK fabbbe32ee
aureleoules:
ACK fabbbe32ee
Tree-SHA512: 5804642658f96a0fb51482ebf3a062bb0f997c1e0527455afa4aceeeb6c1ad139a98b14a7c8a0909daba733a83bdc24fcadad45060ead4be6eb3dc3e66c129e2
33b12e5df6 docs: improve docs where MemPoolLimits is used (stickies-v)
6945853c0b test: use NoLimits() in MempoolIndexingTest (stickies-v)
3a86f24a4c refactor: mempool: use CTxMempool::Limits (stickies-v)
b85af25f87 refactor: mempool: add MemPoolLimits::NoLimits() (stickies-v)
Pull request description:
Mempool currently considers 4 limits regarding ancestor and descendant count and size, which get passed around between functions quite a bit. This PR uses `CTxMemPool::Limits` introduced in https://github.com/bitcoin/bitcoin/pull/25290 to simplify those signatures and callsites.
The purpose of this PR is to improve readability and maintenance, without behaviour change.
As noted in the first commit "refactor: mempool: change MemPoolLimits members to uint", we currently have an underflow issue where a user could pass a negative `-limitancestorsize`, which is eventually cast to an unsigned integer. This behaviour already exists. Because it's orthogonal and to minimize scope, I think this should be fixed in a separate PR.
ACKs for top commit:
hebasto:
ACK 33b12e5df6, I have reviewed the code and it looks OK, I agree it can be merged.
glozow:
reACK 33b12e5df6
Tree-SHA512: 591c6dcee1894f1c3ca28b34a680eeadcf0d40cda92451b4a422c03087b27d682b5e30ba4367abd75a99b5ccb115b7884b0026958d3c7dddab030549db5a4056
01bf4af4f2 docs: fix m_children to be a member of CTxMemPoolEntry (stickies-v)
Pull request description:
Small documentation fix to reflect that `m_children` [is a member](73b61717a9/src/txmempool.h (L99)) of `CTxMemPoolEntry`, not `CTxMemPool`
ACKs for top commit:
hebasto:
ACK 01bf4af4f2, wrong wording was introduced in bitcoin/bitcoin#19478.
glozow:
ACK 01bf4af4f2
Tree-SHA512: b66c43b92fda44682b1f67c43073ca9e133a6dc03cd28253e571e67170531138c20b22ffdb08f312fb2d47a1f869b876611646b54325c8b614d12049befad578
From Russ Yanofsky:
"Agree with all of Marco's points here and think this should be updated
If havePrune and hasAssumedValidChain are both true, better to show
havePrune error message. Assumed-valid error message is vague and not
very actionable. Would suggest "Error loading wallet. Wallet requires
blocks to be downloaded, and software does not currently support loading
wallets while blocks are being downloaded out of order though assumeutxo
snapshots. Wallet should be able to load successfully after node sync
reaches height {block_height}"
Co-authored-by: MacroFake <MarcoFalke@gmail.com>
Co-authored-by: Russ Yanofsky <russ@yanofsky.org>
Instead of having an entire TaprootBuilder which may or may not be
complete, and could potentially have future changes that interact oddly
with taproot tree tuples, have m_tap_tree be just the tuples.
When needed in other a TaprootBuilder for actual use, the tuples will be
added to a a TaprootBuilder that, in the future, can take in whatever
other data is needed as well.
Merging should be checking that the current PSBTOutput doesn't have a
taptree and the other one's is copied over. The original merging had
this inverted and would remove m_tap_tree if the other did not have it.
0f40d65321 refactor: remove duplicate code from BlockAssembler (James O'Beirne)
Pull request description:
Found while reminding myself how transactions are chosen for blocks. Take it or leave it!
ACKs for top commit:
glozow:
ACK 0f40d65321
theStack:
Concept and code-review ACK 0f40d65321
Tree-SHA512: 8a2694e670ce3fe897ab8f64f64c8df5f8487fc1264527a3abbcba0e5b921fb693416497ccd62508295bc33f202c65556b91b6af463acb91aab43138d2492c14
Check `port` options for invalid values (ports are parsed as uint16, so
in practice values >65535 are invalid; port 0 is undefined and therefore
considered invalid too). This allows for an early rejection of faulty
values and an supplying an informative message to the user.
Splits tests in `feature_proxy.py` to cover both invalid `hostname`
and `port` values.
Adds a release-note as previously valid `-port` and `-rpcport` values
can now result in errors.
Forward the validation of the port from `ParseUInt16(...)`.
Consider port 0 as invalid.
Add suitable test for the `SplitHostPort` function.
Add doxygen description to the `SplitHostPort` function.
Since commit f08c9fb0c6 from PR
https://github.com/bitcoin/bitcoin/pull/21726, index
`BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has
also been a race condition in the `coinstatsindex_initial_sync` unit test.
It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the
last connected block to be fully processed, than to be able to return before
prune locks are set, so this switches the order of `m_best_block_index =
block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more
reliable.
Also since commit f08c9fb0c6, there has been a
race condition in the `coinstatsindex_initial_sync` test. Before that commit,
the atomic index best block pointer `m_best_block_index` was updated as the
last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain`
could safely be used in tests to wait for the last `BlockConnected`
notification to be finished before stopping and destroying the index. But
after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer
sufficient, and there is a race between the test shutdown code which destroys
the index object and the new code introduced in that commit calling
`AllowPrune()` and `GetName()` on the index object. Reproducibility
instructions for this are in
https://github.com/bitcoin/bitcoin/issues/25365#issuecomment-1259744133
This commit fixes the `coinstatsindex_initial_sync` race condition, even though
it will require an additional change to silence TSAN false positives,
https://github.com/bitcoin/bitcoin/pull/26188, after it is fixed. So this
partially addresses but does not resolve the bug reporting TSAN errors
https://github.com/bitcoin/bitcoin/issues/25365.
There is no known race condition outside of test code currently, because the
bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not
`BlockUntilSyncedToCurrentChain` to safely shut down.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Co-authored-by: MacroFake <falke.marco@gmail.com>
The (100, 1000000, 1000, 1000000) limits are arbitrarily high and
don't restrict anything, they are just meant to calculate ancestors
properly. Using NoLimits() makes this intent more clear and simplifies
the code.
Simplifies function signatures by removing repetition of all the
ancestor/descendant limits, and increases readability by being
more verbose by naming the limits, while still reducing the LoC.
There are quite a few places in the codebase that require us to
construct a CTxMemPool without limits on ancestors and descendants.
This helper function allows us to get rid of all that duplication.
8a6b6dfcd8 fuzz: pass max fee into ConsumeTxMemPoolEntry (fanquake)
eb15569280 fuzz: add util/mempool/h.cpp (fanquake)
Pull request description:
Moving the heavy (Boost) mempool code out of fuzz/util.h. Means that (for ex) a crypto_common fuzz unit doesn't need to care about seeing endless Boost headers. This results in a ~10% speedup (for me) when compiling the fuzz tests. Your results may vary.
ACKs for top commit:
MarcoFalke:
review ACK 8a6b6dfcd8🍮
Tree-SHA512: 27dc9d9581ac0b1b319cc0dc08fe5f8fbf9269386a5cb23f6fd5d8231bf015ed942ab4414d8001220541be0013756354578ddab1fec607c6fba04daf421bc870
Quoting ryanofsky: "util can be the library for things included in the kernel
which the kernel can depend on, and common can be the library for other code
that needs to be shared internally, but should not be part of the kernel or
shared externally."
7d14577d0f refactor: move DEFAULT_BLOCKFILTERINDEX from val to blockfilterindex (fanquake)
c87d569189 refactor: move DEFAULT_COINSTATSINDEX from validation to coinstatsindex (fanquake)
2bfc1e6aaa refactor: move DEFAULT_TXINDEX from validation to txindex (fanquake)
Pull request description:
Move `*index` default constants out of `validation.h`.
ACKs for top commit:
stickies-v:
re-ACK 7d14577d0f
aureleoules:
ACK 7d14577d0f
Tree-SHA512: 3021db1a63ceb714dee4b91f755d1fb9a6633adb6f1081e34e4179900e7543e3a7b06fe47507d580a3a2caf52f7ede784cb36716d521c76b0404bdc798f0186a
4bee62e9b8 kernel: remove util/bytevectorhash.cpp (fanquake)
Pull request description:
This is no-longer used.
ACKs for top commit:
hebasto:
ACK 4bee62e9b8, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 4d61f87b640ef3c759008631433b3e6d2bd2ac54bbe0b287f32ea1569760048f17a66cfe846b94ec458a7db5d064be6da59299b9280572a3dc649df60760c63f
d0d9cf7aea test: Check external coin effective value is used in CoinSelection (Aurèle Oulès)
76b79c1a17 wallet: Use correct effective value when checking target (Aurèle Oulès)
Pull request description:
Fixes #26185. The following assert failed because it was not checked in the parent function.
2bd9aa5a44/src/wallet/coinselection.cpp (L391)
ACKs for top commit:
glozow:
reACK d0d9cf7aea
furszy:
ACK d0d9cf7a
Tree-SHA512: e126daba1115e9d143f2a582c6953e7ea55e96853b6e819c7744fd7a23668f7d9854681d43ef55d8774655bc54e7e87c1c9fccd746d9e30fbf3caa82ef808ae9
30cc1c6609 refactor: Drop `owns_lock()` call (Hennadii Stepanov)
bff4e068b6 refactor: Do not discard `try_lock()` return value (Hennadii Stepanov)
Pull request description:
Microsoft's C++ Standard Library uses the `[[nodiscard]]` attribute for `try_lock()`.
See: https://github.com/microsoft/STL/blob/main/stl/inc/mutex
This change allows to drop the current suppression for the warning C4838 and helps to prevent the upcoming warning C4858.
See: 539c26c923
Fixes bitcoin/bitcoin#26017.
Split from bitcoin/bitcoin#25819.
ACKs for top commit:
vasild:
ACK 30cc1c6609
Tree-SHA512: ce17404e1c78af4f763129753caf8e5a0e1c91ba398778fe912f9fcc56a847e8112460d1a1a35bf905a593b7d8e0b16c6b099ad74976b67dca5f4f3eda6ff621
