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>
The address of the peer always exists (because addr is a member of
CNode), so it was not possible to pass a nullptr before.
Also remove NET_UNKNOWN, which is unused now.
ff9d961bf3 wallet: Add tracing for sqlite statements (Ryan Ofsky)
Pull request description:
I found sqlite tracing was useful for debugging a test in #27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options.
ACKs for top commit:
achow101:
ACK ff9d961bf3
kevkevinpal:
ACK ff9d961bf3
theStack:
ACK ff9d961bf3
Tree-SHA512: 592fabfab3218cec36c2d00a21cd535fa840daa126ee8440c384952fbb3913180aa3796066c630087e933d6517f19089b867f158e0b737f25283a14799eefb05
I found sqlite tracing was useful for debugging a test in #27790, and thought
it might be helpful in other contexts too, so this PR adds an option to enable
it. Tracing is still disabled by default and only shown with `-debug=walletdb
-loglevel=walletdb:trace` options.
ba616b932c wallet: Add GetPrefixCursor to DatabaseBatch (Andrew Chow)
1d858b055d walletdb: Handle when database keys are empty (Ryan Ofsky)
84b2f353bb walletdb: Consistently clear key and value streams before writing (Andrew Chow)
Pull request description:
Split from #24914 as suggested in https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917
This PR adds a wallet database cursor that gives a view over all of the records beginning with the same prefix.
ACKs for top commit:
ryanofsky:
Code review ACK ba616b932c. Just suggested changes since last review
furszy:
ACK ba616b93
Tree-SHA512: 38a61849f108d8003d28c599b1ad0421ac9beb3afe14c02f1253e7b4efc3d4eef483e32647a820fc6636bca3f9efeff9fe062b6b602e0cded69f21f8b26af544
