Make LoadChainstate return an explicit error when snapshot validation succeeds,
but there is an error trying to replace the background chainstate with the
snapshot chainstate. Previously in this case LoadChainstate would trigger a
shutdown and return INTERRUPTED, now it will return an actual error code.
There's no real change to behavior other than error message being formatted a
little differently.
Motivation for this change is to replace error handling via callbacks with
error handling via return value ahead of
https://github.com/bitcoin/bitcoin/pull/27861
fa8ef7d138 refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation (MarcoFalke)
Pull request description:
This refactor shouldn't change behavior, but may fix compile errors such as https://github.com/bitcoin/bitcoin/pull/27862#issuecomment-1592516184
ACKs for top commit:
achow101:
ACK fa8ef7d138
ryanofsky:
Code review ACK fa8ef7d138. Looks great! Thanks for updating
hebasto:
ACK fa8ef7d138, I have reviewed the code and it looks OK.
Tree-SHA512: 903019962f27b5432b8e3af052b472238ef68d3ee165148c9d2232bf290309075f9f17d8d06c9b5c7fddb89c1a9c3a4c09c6310af01e8561adc0244a30db0857
When processing `CMPCTBLOCK` message, at some moments
we can need to process cmpct block txns, since all messages
are handled by ProcessMessage, we call ProcessMessage
all over again. For this reason, it creates a function called
`ProcessCompactBlockTxns` to process it.
The return type of TranslateArg is std::string, which creates a copy.
Fix this by moving everything into a lambda that takes a reference and
returns a reference.
Also, the format function is called without specifying the namespace it
lives in. Fix this by specifying the namespace. See also:
7a59865793/doc/developer-notes.md (L117-L137).
If -acceptstalefeeestimates option is passed stale fee estimates can now
be read when operating in regtest environments.
Additionally, this commit updates all declarations of the CBlockPolicyEstimator
class to include a the second constructor variable.
This is a refactor as long as no signed integer overflow appears. In
normal operation and absent bugs, signed integer overflow should never
happen in the touched code paths.
The main benefit of this refactor is to drop the file-wide ubsan
suppression unsigned-integer-overflow:txmempool.cpp.
For now, this only changes the internal private representation and the
publicly returned type remains uint64_t.
and drop the util/random dependency on util/setup_common.
This improves code separation and avoids creating a circular dependency if
setup_common needs to call the util/random functions.
cdba23db35 wallet: Document blank flag use in descriptor wallets (Ryan Ofsky)
43310200dc wallet: Ensure that the blank wallet flag is unset after imports (Andrew Chow)
e9379f1ffa rpc, wallet: Include information about blank flag (Andrew Chow)
Pull request description:
The `blank` wallet flag is used to indicate that the wallet intentionally does not have any keys, scripts, or descriptors, and it prevents the automatic generation of those things for such a wallet. Once the wallet contains any of those data, it is unnecessary, and possibly incorrect, to have `blank` set. This PR fixes a few places where this was not properly happening. It also adds a test for this unset behavior.
ACKs for top commit:
S3RK:
reACK cdba23db35
ryanofsky:
Code review ACK cdba23db35. Only change since last review is dropping the commit which makes createwallet RPC set BLANK flag automatically when DISABLE_PRIVATE_KEYS flag is set
Tree-SHA512: 85bc2a9754df0531575d5c8f4ad7e8f38dcd50083dc29b3283dacf56feae842e81f34654c5e1781f2dadb0560ff80e454bbc8ca3b2d1fab1b236499ae9abd7da
d54819d74e scripted-diff: Use datadir from options in chainstatemanager test (TheCharlatan)
Pull request description:
This should make the test less reliant on argument state from the test setup. This is a follow-up PR as requested in https://github.com/bitcoin/bitcoin/pull/27576#discussion_r1224638890.
ACKs for top commit:
achow101:
ACK d54819d74e
MarcoFalke:
lgtm ACK d54819d74e
kevkevinpal:
ACK d54819d74e
ryanofsky:
Code review ACK d54819d74e
Tree-SHA512: 939fde2505c5585d993545a3d05d3a00caec40f860c74fa002caebdf4c1b70e774cfb028a8a8f780525f8968844157d2c568d9f2c8dd5ec32b093173d8644c34
76c5ea703e fuzz: Fix mini_miner_selection running out of coin (Murch)
Pull request description:
Fixes a bug in the mini_miner_selection fuzz test found by fuzzing: It was possible for the mini_miner_selection fuzz test to generated transactions that created fewer new outputs than the two inputs they each spent. If the fuzz seed did so consistently, eventually it would cause a `pop_front()` on an empty available_coins which resulted in undefined behavior.
Fixed per belt-suspender approach:
- assert that available_coins is not empty before generating tx
- generate at least two coins per new tx
- allow building tx with a single input if only one coin is available
ACKs for top commit:
MarcoFalke:
lgtm ACK 76c5ea703e
dergoegge:
reACK 76c5ea703e
Tree-SHA512: 5b7ffd1905a712733ad5364958ad79874dd8c31bd50069b0d3e6f734da0f2d496cb08cbe0afa47115674313e1cb7166a6087f2ccbce289774caddc790583e241
3ef756a5b5 Remove txmempool implicit-integer-sign-change sanitizer suppressions (Hennadii Stepanov)
d2f6d2a95a Use `int32_t` type for most transaction size/weight values (Hennadii Stepanov)
Pull request description:
From bitcoin/bitcoin#23957 which has been incorporated into this PR:
> A file-wide suppression is problematic because it will wave through future violations, potentially bugs.
>
> Fix that by using per-statement casts.
>
> This refactor doesn't change behavior because the now explicit casts were previously done implicitly.
>
> Similar to commit 8b5a4de904
ACKs for top commit:
achow101:
ACK 3ef756a5b5
0xB10C:
ACK 3ef756a5b5. I've focused my testing and code review on the tracepoint related changes. The docs, the test, and the mempool_monitor.py demo script are updated. I ran the `interface_usdt_mempool.py` test and the `mempool_monitor.py` script. The `mempool_monitor.py` output looks correct.
Xekyo:
codereview ACK 3ef756a5b5
ryanofsky:
Code review ACK 3ef756a5b5. Since last review, just rebased with more type changes in test and tracing code
Tree-SHA512: 397407f72165b6fb85ff1794eb1447836c4f903efed1a05d7a9704c88aa9b86f330063964370bbd59f6b5e322e04e7ea8e467805d58dce381e68f7596433330f
This should make the test less reliant on details of the test setup
-BEGIN VERIFY SCRIPT-
sed -i 's/m_args.GetDataDirNet()/chainman.m_options.datadir/g' src/test/validation_chainstatemanager_tests.cpp
-END VERIFY SCRIPT-
7d452d826a test: add coverage for `/deploymentinfo` passing a blockhash (brunoerg)
ce887eaf49 rest: bugfix, fix crash error when calling `/deploymentinfo` (brunoerg)
Pull request description:
Calling `/deploymentinfo` passing a valid blockhash makes bitcoind to crash. It happens because we're pushing a JSON value of type array when it expects type object. See:
```cpp
jsonRequest.params = UniValue(UniValue::VARR);
```
```cpp
jsonRequest.params.pushKV("blockhash", hash_str);
```
This PR fixes it by changing `pushKV` to `push_back` and adds more test coverage.
ACKs for top commit:
achow101:
ACK 7d452d826a
stickies-v:
ACK 7d452d826a
Tree-SHA512: f01551e556aba2380c3eaed0bc59057304302c202d317d7c1eec5f7ef839851f672aed80819a8719cb1cbbad2aad735d6d44314ac7d6d98bff8217f5a16c312b
Fixes a bug in the mini_miner_selection fuzz test found by fuzzing:
It was possible for the mini_miner_selection fuzz test to generated
transactions that created fewer new spendable outputs than the two
inputs they each spend. If the fuzz seed did so consistently, eventually
it would cause a `pop_front()` on an empty available_coins.
Fixed by:
- asserting that available_coins is not empty before generating tx
- allowing to build tx with a single coin if only one is available
When the address is from a network group we already caught,
do a `continue` and try to find another address until conditions
are met or we reach the limit (`nTries`).
61c569ab60 refactor: decouple early return commands from AppInit (furszy)
4927167f85 gui: return EXIT_FAILURE on post-init fatal errors (furszy)
3b2c61e819 Return EXIT_FAILURE on post-init fatal errors (furszy)
3c06926cf2 refactor: index: use `AbortNode` in fatal error helper (Sebastian Falbesoner)
9ddf7e03a3 move ThreadImport ABC error to use AbortNode (furszy)
Pull request description:
It seems odd to return `EXIT_SUCCESS` when the node aborted execution due a fatal internal error
or any post-init problem that triggers an unrequested shutdown.
e.g. blocks or coins db I/O errors, disconnect block failure, failure during thread import (external
blocks loading process error), among others.
ACKs for top commit:
TheCharlatan:
ACK 61c569ab60
ryanofsky:
Code review ACK 61c569ab60
pinheadmz:
ACK 61c569ab60
theStack:
Code-review ACK 61c569ab60
Tree-SHA512: 18a59c3acc1c6d12cbc74a20a401e89659740c6477fccb59070c9f97922dfe588468e9e5eef56c5f395762187c34179a5e3954aa5b844787fa13da2e666c63d3
faa2976a56 Remove mapRelay (MarcoFalke)
fccecd75fe net_processing: relay txs from m_most_recent_block (Anthony Towns)
Pull request description:
`mapRelay` (used to relay announced transactions that are no longer in the mempool) has issues:
* It doesn't have an absolute memory limit, only an implicit one based on the rate of transaction announcements
* <strike>It doesn't have a use-case</strike> EDIT: see below
Fix all issues by removing `mapRelay`.
For more context, on why a transaction may have been removed from the mempool, see c2f2abd0a4/src/txmempool.h (L228-L238)
For my rationale on why it is fine to not relay them:
Reason | | Rationale
-- | -- | --
`EXPIRY` | Expired from mempool | Mempool expiry is by default 2 weeks and can not be less than 1 hour, so a transaction can not be in `mapRelay` while expiring, unless a re-broadcast happened. This should be fine, because the transaction will be re-added to the mempool and potentially announced/relayed on the next re-broadcast.
`SIZELIMIT` | Removed in size limiting | A low fee transaction, which will be relayed by a different peer after `GETDATA_TX_INTERVAL` or after we sent a `notfound` message. Assuming it ever made it to another peer, otherwise it will happen on re-broadcast (same as with `EXPIRY` above).
`REORG` | Removed for reorganization | Block races are rare, so reorgs should be rarer. Also, the transaction is likely to be re-accepted via the `disconnectpool` later on. If not, it seems fine to let the originating wallet deal with rebroadcast in this case.
`BLOCK` | Removed for block | EDIT: Needed for compact block relay, see https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544047433
`CONFLICT` | Removed for conflict with in-block transaction | The peer won't be able to add the tx to the mempool anyway, unless it is on a different block, in which case it seems fine to let the originating wallet take care of the rebroadcast (if needed).
`REPLACED` | Removed for replacement | EDIT: Also needed for compact block relay, see https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255 ?
ACKs for top commit:
sdaftuar:
ACK faa2976a56
ajtowns:
ACK faa2976a56
glozow:
code review ACK faa2976a56
Tree-SHA512: 64ae3e387b001bf6bd5b6c938e7317f4361f9bc0b8cc5d8f63a16cda2408d2f634a22f8157dfcd8957502ef358208292ec91e7d70c9c2d8a8c47cc0114ecfebd
Cleaned up the init flow to make it more obvious when
the 'exit_status' value will and won't be returned.
This is because it was confusing that `AppInit` was
returning true under two different circumstances:
1) When bitcoind was launched only to retrieve the "-help"
or "-version" information. In this case, the app was
not initialized.
2) When the user triggers a shutdown. In this case,
the app was fully initialized.
It seems odd to return `EXIT_SUCCESS` when the node aborted
execution due a fatal internal error or any post-init problem
that triggers an unrequested shutdown.
e.g. blocks or coins db I/O errors, disconnect block failure,
failure during thread import (external blocks loading process
error), among others.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
11bb31c1c4 p2p: "skip netgroup diversity of new connections for tor/i2p/cjdns" follow-up (Jon Atack)
Pull request description:
In #27374 the role of the `setConnected` data structure in `CConnman::ThreadOpenConnections` changed from the set of outbound peer netgroups to those of outbound IPv4/6 peers only.
In accordance with the changed semantics, this pull fixes a code comment regarding feeler connections and updates the naming of `setConnected` to `outbound_ipv46_peer_netgroups`.
Addresses https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1167172725.
ACKs for top commit:
mzumsande:
Code Review ACK 11bb31c1c4
vasild:
ACK 11bb31c1c4
ryanofsky:
Code review ACK 11bb31c1c4
Tree-SHA512: df9151a6cce53c279e549683a9f30fdc23d513dc664cfee1cf0eb8ec80b2848d32c80a92cc0a9f47d967f305864975ffb339fe0eaa80bc3bef1b28406419eb96
Deduplicates code in the `FatalError` template function by using
`AbortNode` which does the exact same thing if called without any user
message (i.e. without second parameter specified). The template is still
kept for ease-of-use w.r.t. not having to call `tfm::format(...)` at the
call-side each time, and also to keep the diff minimal.
Move wallet flags loading to its own function in WalletBatch
The return value is changed to be TOO_NEW rather than CORRUPT when
unknown flags are found.
71200ac390 [fuzz] Only check duplicate coinbase script when block was valid (dergoegge)
Pull request description:
Partially revert #27780, because moving the duplicate coinbase check out of the `was_valid` branch leads to non-bug crashes in the fuzz target.
For context and further explanation see: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=59516
ACKs for top commit:
MarcoFalke:
nice lgtm ACK 71200ac390
Tree-SHA512: 8c38e5ff9de6331016b9a0c5e435d007d46186151b04c09085f617bb31627a28ad56678066fe152372a3ad8656f026439e3e2f9ee61d7ef588072aef8124eaa3
67b7fecacd [mempool] clear mapDeltas entry if prioritisetransaction sets delta to 0 (glozow)
c1061acb9d [functional test] prioritisation is not removed during replacement and expiry (glozow)
0e5874f0b0 [functional test] getprioritisedtransactions RPC (glozow)
99f8046829 [rpc] add getprioritisedtransactions (glozow)
9e9ca36c80 [mempool] add GetPrioritisedTransactions (glozow)
Pull request description:
Add an RPC to get prioritised transactions (also tells you whether the tx is in mempool or not), helping users clean up `mapDeltas` manually. When `CTxMemPool::PrioritiseTransaction` sets a delta to 0, remove the entry from `mapDeltas`.
Motivation / Background
- `mapDeltas` entries are never removed from mapDeltas except when the tx is mined in a block or conflicted.
- Mostly it is a feature to allow `prioritisetransaction` for a tx that isn't in the mempool {yet, anymore}. A user can may resbumit a tx and it retains its priority, or mark a tx as "definitely accept" before it is seen.
- Since #8448, `mapDeltas` is persisted to mempool.dat and loaded on restart. This is also good, otherwise we lose prioritisation on restart.
- Note the removal due to block/conflict is only done when `removeForBlock` is called, i.e. when the block is received. If you load a mempool.dat containing `mapDeltas` with transactions that were mined already (e.g. the file was saved prior to the last few blocks), you don't delete them.
- Related: #4818 and #6464.
- There is no way to query the node for not-in-mempool `mapDeltas`. If you add a priority and forget what the value was, the only way to get that information is to inspect mempool.dat.
- Calling `prioritisetransaction` with an inverse value does not remove it from `mapDeltas`, it just sets the value to 0. It disappears on a restart (`LoadMempool` checks if delta is 0), but that might not happen for a while.
Added together, if a user calls `prioritisetransaction` very regularly and not all those transactions get mined/conflicted, `mapDeltas` might keep lots of entries of delta=0 around. A user should clean up the not-in-mempool prioritisations, but that's currently difficult without keeping track of what those txids/amounts are.
ACKs for top commit:
achow101:
ACK 67b7fecacd
theStack:
Code-review ACK 67b7fecacd
instagibbs:
code review ACK 67b7fecacd
ajtowns:
ACK 67b7fecacd code review only, some nits
Tree-SHA512: 9df48b622ef27f33db1a2748f682bb3f16abe8172fcb7ac3c1a3e1654121ffb9b31aeaad5570c4162261f7e2ff5b5912ddc61a1b8beac0e9f346a86f5952260a
Stop advertising
1) our i2p/onion address to peers from other networks
2) Local addresses of non-privacy networks to i2p/onion peers
Doing so could lead to fingerprinting ourselves.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
