Commit graph

24205 commits

Author SHA1 Message Date
furszy
a72af2e833
bench: disable birth time block skip for wallet_create_tx.cpp
As the benchmarks inside wallet_create_tx.cpp assert the
wallet balance at the end, they require all
blocks to be scanned by the wallet. So, we need
to ensure that no blocks are skipped by the recently
added wallet birth time functionality.

This just means setting the wallet birthtime to the
genesis block time. So the wallet is always older than
any new block.
2023-06-16 21:00:20 -03:00
Andrew Chow
f0758d8a66
Merge bitcoin/bitcoin#27757: rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet
5524fa00fa doc: add release note about removal of `deprecatedrpc=walletwarningfield` flag (Sebastian Falbesoner)
5c77db7354 Restorewallet/createwallet help documentation fixups/improvements (Jon Atack)
a00ae31fcc rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet (Sebastian Falbesoner)

Pull request description:

  The "warning" string field for wallet creating/loading RPCs (`createwallet`, `loadwallet`, `unloadwallet` and `restorewallet`) has been deprecated with the configuration option `-deprecatedrpc=walletwarningfield` in PR #27279 (released in v25.0). For the next release v26.0, the field and the configuration option can be removed.

ACKs for top commit:
  achow101:
    ACK 5524fa00fa
  jonatack:
    ACK 5524fa00fa

Tree-SHA512: 8212f72067d08095304018b8a95d2ebef630004b65123483fbbfb078cc5709c2d825bbc35b16ea5f6b28ae7377347382d7e9afaf7bdbf0575d2c229d970784de
2023-06-16 15:11:44 -04:00
Andrew Chow
5b8e07725d
Merge bitcoin/bitcoin#27892: refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation
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
2023-06-15 14:29:55 -04:00
MarcoFalke
fa8ef7d138
refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation
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).
2023-06-15 16:21:29 +02:00
brunoerg
162602b208 fuzz: wallet, add target for fees 2023-06-14 11:20:39 -03:00
Ryan Ofsky
6663c802fe
Merge bitcoin/bitcoin#25634: wallet, tests: Expand and test when the blank wallet flag should be un/set
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
2023-06-14 09:30:39 -04:00
Andrew Chow
427853ab49
Merge bitcoin/bitcoin#27876: test: (refactor) Use datadir from options in chainstatemanager test
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
2023-06-13 16:28:16 -04:00
Ryan Ofsky
cdba23db35 wallet: Document blank flag use in descriptor wallets 2023-06-13 15:11:41 -04:00
Andrew Chow
43310200dc wallet: Ensure that the blank wallet flag is unset after imports 2023-06-13 15:11:41 -04:00
fanquake
da494186f2
Merge bitcoin/bitcoin#27806: fuzz: Fix mini_miner_selection running out of coin
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
2023-06-13 17:08:07 +01:00
Andrew Chow
58b36fc303
Merge bitcoin/bitcoin#23962: Use int32_t type for most transaction size/weight values
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
2023-06-13 10:37:25 -04:00
TheCharlatan
d54819d74e
scripted-diff: Use datadir from options in chainstatemanager test
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-
2023-06-13 13:52:42 +02:00
Andrew Chow
d80348ccb6
Merge bitcoin/bitcoin#27853: rest: bugfix, fix crash error when calling /deploymentinfo
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
2023-06-12 18:34:42 -04:00
Hennadii Stepanov
3ef756a5b5
Remove txmempool implicit-integer-sign-change sanitizer suppressions 2023-06-12 19:48:47 +01:00
Hennadii Stepanov
d2f6d2a95a
Use int32_t type for most transaction size/weight values
This change gets rid of a few casts and makes the following commit diff
smaller.
2023-06-12 19:47:19 +01:00
Murch
76c5ea703e
fuzz: Fix mini_miner_selection running out of coin
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
2023-06-12 14:19:53 -04:00
Ryan Ofsky
c92fd63886
Merge bitcoin/bitcoin#27708: Return EXIT_FAILURE on post-init fatal errors
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
2023-06-12 12:54:49 -04:00
fanquake
361a0c00b3
Merge bitcoin/bitcoin#27783: Add public Boost headers explicitly
2484cacb7a Add public Boost headers explicitly (Hennadii Stepanov)
fade2adb5b test: Avoid `BOOST_ASSERT` macro (Hennadii Stepanov)

Pull request description:

  To check symbols in the code base, run:
  ```
  git grep boost::multi_index::identity
  git grep boost::multi_index::indexed_by
  git grep boost::multi_index::tag
  git grep boost::make_tuple
  ```

  Hoping on the absence of conflicts with top-prio PRs :)

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 2484cacb7a
  TheCharlatan:
    ACK 2484cacb7a

Tree-SHA512: d122ab028eee76ee1c4609ed51ec8db0c8c768edcc2ff2c0e420a48e051aa71e99748cdb5d22985ae6d97c808c77c1a27561f0715f77b256f74c1c310b37694c
2023-06-12 16:53:16 +01:00
brunoerg
ce887eaf49 rest: bugfix, fix crash error when calling /deploymentinfo 2023-06-12 10:24:14 -03:00
fanquake
6f5f37eefd
Merge bitcoin/bitcoin#27357: validation: Move warningcache to ChainstateManager and rename to m_warningcache
552684976b validation: Move warningcache to ChainstateManager (dimitaracev)

Pull request description:

  Removes `warningcache`  and moves it to `ChainstateManager`. Also removes the respective `TODO`  completely.

ACKs for top commit:
  ajtowns:
    ACK 552684976b
  dimitaracev:
    > ACK [5526849](552684976b)
  TheCharlatan:
    ACK 552684976b
  ryanofsky:
    Code review ACK 552684976b

Tree-SHA512: 6869bd7aa4f0b59324e12eb8e3df47f2c9a3f3b0d9b7d45857426ec9e8b71c5573bdcf71db822f8c10aff7d8679a00a4bedc7a256c28f325e744e5d7267b41e9
2023-06-12 13:20:18 +01:00
fanquake
fbe48f97df
Merge bitcoin/bitcoin#27625: p2p: Stop relaying non-mempool txs
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
2023-06-12 10:50:27 +01:00
furszy
61c569ab60
refactor: decouple early return commands from AppInit
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.
2023-06-10 11:10:29 -03:00
furszy
4927167f85
gui: return EXIT_FAILURE on post-init fatal errors 2023-06-10 11:10:29 -03:00
furszy
3b2c61e819
Return EXIT_FAILURE on post-init fatal errors
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>
2023-06-09 17:52:23 -03:00
Ryan Ofsky
153a6882f4
Merge bitcoin/bitcoin#27576: kernel: Remove args, settings, chainparams, chainparamsbase from kernel library
db77f87c63 scripted-diff: move settings to common namespace (TheCharlatan)
c27e4bdc35 move-only: Move settings to the common library (TheCharlatan)
c2dae5d7d8 kernel: Remove chainparams, chainparamsbase, args, settings from kernel library (TheCharlatan)
05870b1c92 refactor: Remove gArgs access from validation.cpp (TheCharlatan)
8789b11114 refactor: Add path argument to FindSnapshotChainstateDir (TheCharlatan)
ef95be334f refactor: Add stop_at_height option in ChainstateManager (TheCharlatan)

Pull request description:

  This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".

  ---

  This completes the removal of the node's chainparams, chainparamsbase, args and settings files and their respective classes from the kernel library. This is the last pull request in a long series working towards decoupling the `ArgsManager` and the `gArgs` global from kernel code. These prior pull requests are: https://github.com/bitcoin/bitcoin/pull/26177 https://github.com/bitcoin/bitcoin/pull/27125 https://github.com/bitcoin/bitcoin/pull/25527 https://github.com/bitcoin/bitcoin/pull/25487 https://github.com/bitcoin/bitcoin/pull/25290

ACKs for top commit:
  MarcoFalke:
    lgtm ACK db77f87c63 🍄
  hebasto:
    ACK db77f87c63, I have reviewed the code and it looks OK.
  ryanofsky:
    Code review ACK db77f87c63. Looks great!

Tree-SHA512: cbfbd705d056f2f10f16810d4f869eb152362fff2c5ddae5e1ac6785deae095588e52ad48b29d921962b085e51de1e0ecab6e50f46149ffe3c16250608a2c93a
2023-06-09 14:58:49 -04:00
Ryan Ofsky
456af7a955
Merge bitcoin/bitcoin#27467: p2p: skip netgroup diversity follow-up
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
2023-06-09 14:21:19 -04:00
Sebastian Falbesoner
3c06926cf2
refactor: index: use AbortNode in fatal error helper
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.
2023-06-08 16:38:36 -03:00
furszy
9ddf7e03a3
move ThreadImport ABC error to use AbortNode
'StartShutdown' should only be used for user requested
shutdowns. Internal errors that cause a shutdown should
use 'AbortNode'.
2023-06-08 16:38:36 -03:00
Andrew Chow
e9379f1ffa rpc, wallet: Include information about blank flag
This allows us to test that the blank flag is being set appropriately.
2023-06-08 07:29:00 -04:00
MarcoFalke
faa2976a56
Remove mapRelay 2023-06-08 11:52:30 +02:00
Anthony Towns
fccecd75fe net_processing: relay txs from m_most_recent_block 2023-06-08 11:52:08 +02:00
fanquake
2026301405
Merge bitcoin/bitcoin#27810: fuzz: Partially revert #27780
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
2023-06-07 15:55:36 +02:00
Andrew Chow
1af72e728d
Merge bitcoin/bitcoin#27501: mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0
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
2023-06-07 03:29:05 -04:00
fanquake
f4a8269dfc
Merge bitcoin/bitcoin#27801: wallet: Add tracing for sqlite statements
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
2023-06-05 10:51:08 +01:00
Jon Atack
5c77db7354 Restorewallet/createwallet help documentation fixups/improvements 2023-06-04 23:53:26 +02:00
Sebastian Falbesoner
a00ae31fcc rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet
Co-authored-by: Jon Atack <jon@atack.com>
2023-06-04 23:53:26 +02:00
dergoegge
71200ac390 [fuzz] Only check duplicate coinbase script when block was valid 2023-06-03 15:37:11 +02:00
Ryan Ofsky
ff9d961bf3 wallet: Add tracing for sqlite statements
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.
2023-06-02 16:47:33 -04:00
fanquake
7f2019755d
Merge bitcoin/bitcoin#27790: walletdb: Add PrefixCursor
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
2023-06-02 17:00:19 +01:00
fanquake
436c185b05
Merge bitcoin/bitcoin#27256: refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it
cfbc8a623b refactor: rpc: hide and rename ParseNonRFCJSONValue() (stickies-v)
6c8bde6d54 test: move coverage on ParseNonRFCJSONValue() to UniValue::read() (stickies-v)

Pull request description:

  Follow-up to https://github.com/bitcoin/bitcoin/pull/26612#issuecomment-1453623741. As per https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059, `ParseNonRFCJSONValue()` is no longer necessary and we can use `UniValue::read()` directly:

  > IIRC before that PR UniValue::read could only parse JSON object and array values, but now it will parse string/number/boolean/null values as well. laanwj pointed this out in https://github.com/bitcoin/bitcoin/issues/9028#issuecomment-257885368

  The implementation of `ParseNonRFCJSONValue()` was already [simplified in #26612](https://github.com/bitcoin/bitcoin/pull/26612/files#diff-84c7a7f36362b9724c31e5dec9879b2f81eae0d0addbc9c0933c3558c577de65R259-R263)  and [test coverage updated](https://github.com/bitcoin/bitcoin/pull/26612/files#diff-fc0f86b6c3bb23b0e983bcf79d7546d1c9eaa15d6e4d8a7b03b5b85955f585abR292-R312) to ensure behaviour didn't change.

  To avoid code duplication, we keep the function to throw on invalid input data but rename it to `Parse()` and remove it from the header.

  The existing test coverage we had on `ParseNonRFCJSONValue()` is moved over to `UniValue::read()`.

ACKs for top commit:
  ryanofsky:
    Code review ACK cfbc8a623b. Only change since last review is adding a new test

Tree-SHA512: 89be959d2353af7ace0c1348ba1600b9ac1d3c7b3cf7f0b59f6e005b7fb9d695ce3e8720e1be3cf77fe7e318a4017c880df108928e7179ec50447583d13bc781
2023-06-02 16:18:11 +01:00
glozow
6a560aceb7
Merge bitcoin/bitcoin#27803: Fuzz: Mitigate timeout in CalculateTotalBumpFees
5d718f6913 Mitigate timeout in CalculateTotalBumpFees (Murch)

Pull request description:

  The slow fuzz seed described in #27799 was just slower than expected, not an endless loop. Ensuring that every anscestor is only processed once speeds up the termination of the graph traversal.

  Fixes #27799

ACKs for top commit:
  glozow:
    ACK 5d718f6913

Tree-SHA512: f3c7cd2ef6716332136c75b43f6d54ce920be6f546a11bbf92b1fd65575607c42cc24b319691d86d0db038335636ba12b6387383a184f1589a8d71d1180f194f
2023-06-02 10:59:35 +01:00
fanquake
83c7269965
Merge bitcoin/bitcoin#27800: streams: Drop confusing DataStream::Serialize method and << operator
5cd0717a54 streams: Drop confusing DataStream::Serialize method and << operator (Ryan Ofsky)

Pull request description:

  DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes.

  Having this inconsistency is not necessary and could be confusing (see https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030) so this PR just drops the DataStream::Serialize method.

ACKs for top commit:
  furszy:
    lgtm ACK 5cd0717a
  MarcoFalke:
    lgtm ACK 5cd0717a54 🌙

Tree-SHA512: 49dd117de266f091a5336b13a91c5d8658abe1b3a0a9c51c8b5f6a2e0e814781b73afc39256353e79dade603a8a2761e8536716d1a48499720c266f4500477e2
2023-06-02 10:28:01 +01:00
Murch
5d718f6913
Mitigate timeout in CalculateTotalBumpFees
The slow fuzz seed described in #27799 was just slower than expected,
not an endless loop. Ensuring that every anscestor is only processed
once speeds up the termination of the graph traversal.

Fixes #27799
2023-06-01 18:04:44 -04:00
Andrew Chow
34ac3f438a
Merge bitcoin/bitcoin#26485: RPC: Accept options as named-only parameters
2cd28e9fef rpc: Add check for unintended option/parameter name clashes (Ryan Ofsky)
95d7de0964 test: Update python tests to use named parameters instead of options objects (Ryan Ofsky)
96233146dd RPC: Allow RPC methods accepting options to take named parameters (Ryan Ofsky)
702b56d2a8 RPC: Add add OBJ_NAMED_PARAMS type (Ryan Ofsky)

Pull request description:

  Allow RPC methods which take an `options` parameter (`importmulti`, `listunspent`, `fundrawtransaction`, `bumpfee`, `send`, `sendall`, `walletcreatefundedpsbt`, `simulaterawtransaction`), to accept the options as named parameters, without the need for nested JSON objects.

  This makes it possible to make calls like:

  ```sh
  src/bitcoin-cli -named bumpfee txid fee_rate=10
  ```

  instead of

  ```sh
  src/bitcoin-cli -named bumpfee txid options='{"fee_rate": 10}'
  ```

  RPC help is also updated to show options as top level named arguments instead of as nested objects.

  <details><summary>diff</summary>
  <p>

  ```diff
  @@ -15,16 +15,17 @@

   Arguments:
   1. txid                           (string, required) The txid to be bumped
  -2. options                        (json object, optional)
  +2. options                        (json object, optional) Options object that can be used to pass named arguments, listed below.
  +
  +Named Arguments:
  -     {
  -       "conf_target": n,          (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks
  +conf_target                       (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks

  -       "fee_rate": amount,        (numeric or string, optional, default=not set, fall back to wallet fee estimation)
  +fee_rate                          (numeric or string, optional, default=not set, fall back to wallet fee estimation)
                                     Specify a fee rate in sat/vB instead of relying on the built-in fee estimator.
                                     Must be at least 1.000 sat/vB higher than the current transaction fee rate.
                                     WARNING: before version 0.21, fee_rate was in BTC/kvB. As of 0.21, fee_rate is in sat/vB.

  -       "replaceable": bool,       (boolean, optional, default=true) Whether the new transaction should still be
  +replaceable                       (boolean, optional, default=true) Whether the new transaction should still be
                                     marked bip-125 replaceable. If true, the sequence numbers in the transaction will
                                     be left unchanged from the original. If false, any input sequence numbers in the
                                     original transaction that were less than 0xfffffffe will be increased to 0xfffffffe
  @@ -32,11 +33,10 @@
                                     still be replaceable in practice, for example if it has unconfirmed ancestors which
                                     are replaceable).

  -       "estimate_mode": "str",    (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
  +estimate_mode                     (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
                                     "unset"
                                     "economical"
                                     "conservative"
  -     }

   Result:
   {                    (json object)
  ```

  </p>
  </details>

  **Review suggestion:** To understand this PR, it is probably easiest to review the commits in reverse order because the last commit shows the external API changes, the middle commit shows the internal API changes, and the first commit contains the low-level implementation.

ACKs for top commit:
  achow101:
    ACK 2cd28e9fef

Tree-SHA512: 50f6e78fa622826dab3f810400d8c1a03a98a090b1f2fea79729c58ad8cff955554bd44c2a5975f62a526b900dda68981862fd7d7d05c17f94f5b5d847317436
2023-06-01 15:30:31 -04:00
Andrew Chow
ba616b932c wallet: Add GetPrefixCursor to DatabaseBatch
In order to get records beginning with a prefix, we will need a cursor
specifically for that prefix. So add a GetPrefixCursor function and
DatabaseCursor classes for dealing with those prefixes.

Tested on each supported db engine.

1) Write two different key->value elements to db.
2) Create a new prefix cursor and walk-through every returned element,
   verifying that it gets parsed properly.
3) Try to move the cursor outside the filtered range: expect failure
   and flag complete=true.

Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
Co-Authored-By: furszy <matiasfurszyfer@protonmail.com>
2023-06-01 13:09:08 -04:00
Ryan Ofsky
5cd0717a54 streams: Drop confusing DataStream::Serialize method and << operator
DataStream Serialize method has surprising behavior because it just serializes
raw bytes without a length prefix. When you serialize a string or vector, a
length prefix is serialized before the raw object contents so the object can be
unambiguously deserialized later. But DataStreams don't support deserializing
at all and just dump the raw bytes.

Having this inconsistency is not necessary and could be confusing (see
https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030) so this
PR just drops the DataStream::Serialize method.
2023-06-01 10:27:33 -04:00
Ryan Ofsky
1d858b055d walletdb: Handle when database keys are empty 2023-05-31 15:24:06 -04:00
Andrew Chow
84b2f353bb walletdb: Consistently clear key and value streams before writing
Before writing data to the output key and value streams, make sure they
are cleared.
2023-05-31 15:17:05 -04:00
Andrew Chow
3a83d4417b
Merge bitcoin/bitcoin#27720: index: prevent race by calling 'CustomInit' prior setting 'synced' flag
3126454dcf index: prevent race by calling 'CustomInit' prior setting 'synced' flag (furszy)

Pull request description:

  Decoupled from #27607.

  Fixed a potential race condition in master (not possible so far) that could become an actual issue soon.
  Where the index's  `CustomAppend` method could be called (from `BlockConnected`) before its
  `CustomInit` method, causing the index to try to update itself before it is initialized.

  This could happen because we set the index `m_synced` flag (which enables `BlockConnected` events)
  before calling to the child class init function (`CustomInit`). So, for example, the block filter index could
  process a block before initialize the next filter position field and end up overwriting the first stored filter.

  This race was introduced in bef4e405f3 from https://github.com/bitcoin/bitcoin/pull/25494.

ACKs for top commit:
  achow101:
    ACK 3126454dcf
  mzumsande:
    Code review ACK 3126454dcf
  TheCharlatan:
    Nice, ACK 3126454dcf

Tree-SHA512: 7a53fed1d2035cb4c1f331d6ee9f92d499b6cbb618ea534c6440f5a45ff9b3ac4dcff5fb4b88937f45a0be249e3a9c6dc6c3ac77180f12ae25fc56856ba39735
2023-05-31 13:56:28 -04:00
Hennadii Stepanov
2484cacb7a
Add public Boost headers explicitly 2023-05-31 15:43:01 +01:00