Age | Commit message (Collapse) | Author |
|
|
|
wallet flag should be un/set
cdba23db353a1beff831ff4fc83d01ed64e8c2a9 wallet: Document blank flag use in descriptor wallets (Ryan Ofsky)
43310200dce8d450ae5808824af788cefaa5d6db wallet: Ensure that the blank wallet flag is unset after imports (Andrew Chow)
e9379f1ffa7a4eebce397f1150317e840655e021 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 cdba23db353a1beff831ff4fc83d01ed64e8c2a9
ryanofsky:
Code review ACK cdba23db353a1beff831ff4fc83d01ed64e8c2a9. 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
|
|
|
|
chainstatemanager test
d54819d74e04e6105c1f0362755f5bcfa845eefd 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 d54819d74e04e6105c1f0362755f5bcfa845eefd
MarcoFalke:
lgtm ACK d54819d74e04e6105c1f0362755f5bcfa845eefd
kevkevinpal:
ACK https://github.com/bitcoin/bitcoin/commit/d54819d74e04e6105c1f0362755f5bcfa845eefd
ryanofsky:
Code review ACK d54819d74e04e6105c1f0362755f5bcfa845eefd
Tree-SHA512: 939fde2505c5585d993545a3d05d3a00caec40f860c74fa002caebdf4c1b70e774cfb028a8a8f780525f8968844157d2c568d9f2c8dd5ec32b093173d8644c34
|
|
|
|
|
|
76c5ea703e77d580b6962e60398f4988cbd9b58b 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 76c5ea703e77d580b6962e60398f4988cbd9b58b
dergoegge:
reACK 76c5ea703e77d580b6962e60398f4988cbd9b58b
Tree-SHA512: 5b7ffd1905a712733ad5364958ad79874dd8c31bd50069b0d3e6f734da0f2d496cb08cbe0afa47115674313e1cb7166a6087f2ccbce289774caddc790583e241
|
|
size/weight values
3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f Remove txmempool implicit-integer-sign-change sanitizer suppressions (Hennadii Stepanov)
d2f6d2a95a9f6c1632c1ed3b5b5b67a49eb71d6b 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 8b5a4de904b414fb3a818732cd0a2c90b91bc275
ACKs for top commit:
achow101:
ACK 3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f
0xB10C:
ACK 3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f. 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 3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f
ryanofsky:
Code review ACK 3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f. 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-
|
|
`/deploymentinfo`
7d452d826a7056411077b870efc3872bb2fa45e4 test: add coverage for `/deploymentinfo` passing a blockhash (brunoerg)
ce887eaf4917c337b21aa2e7811804ce003d36be 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 7d452d826a7056411077b870efc3872bb2fa45e4
stickies-v:
ACK 7d452d826a7056411077b870efc3872bb2fa45e4
Tree-SHA512: f01551e556aba2380c3eaed0bc59057304302c202d317d7c1eec5f7ef839851f672aed80819a8719cb1cbbad2aad735d6d44314ac7d6d98bff8217f5a16c312b
|
|
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
|
|
|
|
This change gets rid of a few casts and makes the following commit diff
smaller.
|
|
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`).
|
|
61c569ab6069d04079a0831468eb713983919636 refactor: decouple early return commands from AppInit (furszy)
4927167f855f8ed3bbf6d2766f61229f742e632a gui: return EXIT_FAILURE on post-init fatal errors (furszy)
3b2c61e8198bcefb1c2343caff1d705951026cc4 Return EXIT_FAILURE on post-init fatal errors (furszy)
3c06926cf21dcca3074ef51506f556b2286c299b refactor: index: use `AbortNode` in fatal error helper (Sebastian Falbesoner)
9ddf7e03a35592617a016418fd320cc93c8d1abd 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 61c569ab6069d04079a0831468eb713983919636
ryanofsky:
Code review ACK 61c569ab6069d04079a0831468eb713983919636
pinheadmz:
ACK 61c569ab6069d04079a0831468eb713983919636
theStack:
Code-review ACK 61c569ab6069d04079a0831468eb713983919636
Tree-SHA512: 18a59c3acc1c6d12cbc74a20a401e89659740c6477fccb59070c9f97922dfe588468e9e5eef56c5f395762187c34179a5e3954aa5b844787fa13da2e666c63d3
|
|
2484cacb7a6367b24e924dba0825c843b1dfc1c3 Add public Boost headers explicitly (Hennadii Stepanov)
fade2adb5bb4ce9753e7f25da5fb1521f2f503ec 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 2484cacb7a6367b24e924dba0825c843b1dfc1c3
TheCharlatan:
ACK 2484cacb7a6367b24e924dba0825c843b1dfc1c3
Tree-SHA512: d122ab028eee76ee1c4609ed51ec8db0c8c768edcc2ff2c0e420a48e051aa71e99748cdb5d22985ae6d97c808c77c1a27561f0715f77b256f74c1c310b37694c
|
|
|
|
ChainstateManager and rename to m_warningcache
552684976b6df34ce563458f73812e6e494e3b0e 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 552684976b6df34ce563458f73812e6e494e3b0e
dimitaracev:
> ACK [5526849](https://github.com/bitcoin/bitcoin/commit/552684976b6df34ce563458f73812e6e494e3b0e)
TheCharlatan:
ACK 552684976b6df34ce563458f73812e6e494e3b0e
ryanofsky:
Code review ACK 552684976b6df34ce563458f73812e6e494e3b0e
Tree-SHA512: 6869bd7aa4f0b59324e12eb8e3df47f2c9a3f3b0d9b7d45857426ec9e8b71c5573bdcf71db822f8c10aff7d8679a00a4bedc7a256c28f325e744e5d7267b41e9
|
|
faa2976a56ea7cdfd77ce2580a89ce493b57b5d4 Remove mapRelay (MarcoFalke)
fccecd75fed50a59ec4d54d6dc9bd9a406ea6b30 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 https://github.com/bitcoin/bitcoin/blob/c2f2abd0a4f4bd18bfca41b632d21d803479f3f4/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 faa2976a56ea7cdfd77ce2580a89ce493b57b5d4
ajtowns:
ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4
glozow:
code review ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4
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>
|
|
chainparamsbase from kernel library
db77f87c6365cb5f414036d6bfb1a12705772028 scripted-diff: move settings to common namespace (TheCharlatan)
c27e4bdc35bc7cedd1ee07e98a52c230241120d1 move-only: Move settings to the common library (TheCharlatan)
c2dae5d7d89634fbd771755ce3909719f5462f63 kernel: Remove chainparams, chainparamsbase, args, settings from kernel library (TheCharlatan)
05870b1c92f39d90e5ba6e0caf2f6c2b37955528 refactor: Remove gArgs access from validation.cpp (TheCharlatan)
8789b11114b4bd6c7ee727dffbc75a6bdf20dd27 refactor: Add path argument to FindSnapshotChainstateDir (TheCharlatan)
ef95be334f3aec671346372b64606e0fd390979a 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 db77f87c6365cb5f414036d6bfb1a12705772028 🍄
hebasto:
ACK db77f87c6365cb5f414036d6bfb1a12705772028, I have reviewed the code and it looks OK.
ryanofsky:
Code review ACK db77f87c6365cb5f414036d6bfb1a12705772028. Looks great!
Tree-SHA512: cbfbd705d056f2f10f16810d4f869eb152362fff2c5ddae5e1ac6785deae095588e52ad48b29d921962b085e51de1e0ecab6e50f46149ffe3c16250608a2c93a
|
|
11bb31c1c43b5da36ca8509b5747abfb3278ffcd 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 11bb31c1c43b5da36ca8509b5747abfb3278ffcd
vasild:
ACK 11bb31c1c43b5da36ca8509b5747abfb3278ffcd
ryanofsky:
Code review ACK 11bb31c1c43b5da36ca8509b5747abfb3278ffcd
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.
|
|
'StartShutdown' should only be used for user requested
shutdowns. Internal errors that cause a shutdown should
use 'AbortNode'.
|
|
This allows us to test that the blank flag is being set appropriately.
|
|
the data push
|
|
|
|
|
|
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.
|
|
Move minversion loading to its own function in WalletBatch
|
|
71200ac390fe5c10f088cbe8053b010b515757b1 [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 71200ac390fe5c10f088cbe8053b010b515757b1
Tree-SHA512: 8c38e5ff9de6331016b9a0c5e435d007d46186151b04c09085f617bb31627a28ad56678066fe152372a3ad8656f026439e3e2f9ee61d7ef588072aef8124eaa3
|
|
delete a mapDeltas entry when delta==0
67b7fecacd0489809690982c89ba2d0acdca938c [mempool] clear mapDeltas entry if prioritisetransaction sets delta to 0 (glozow)
c1061acb9d502cdf8c6996c818d9a8a281cbe40c [functional test] prioritisation is not removed during replacement and expiry (glozow)
0e5874f0b06114d9b077e0ff582915e4f83059e6 [functional test] getprioritisedtransactions RPC (glozow)
99f8046829f699ff2eace266aa8cea1d9f7cb65a [rpc] add getprioritisedtransactions (glozow)
9e9ca36c80013749faaf2aa777d52bd07d9d24ec [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 67b7fecacd0489809690982c89ba2d0acdca938c
theStack:
Code-review ACK 67b7fecacd0489809690982c89ba2d0acdca938c
instagibbs:
code review ACK 67b7fecacd0489809690982c89ba2d0acdca938c
ajtowns:
ACK 67b7fecacd0489809690982c89ba2d0acdca938c 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.
|
|
Access to CNode will be needed in the following commits.
|
|
ff9d961bf38b24f8f931dcf66799cbc468e473df 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 ff9d961bf38b24f8f931dcf66799cbc468e473df
kevkevinpal:
ACK https://github.com/bitcoin/bitcoin/commit/ff9d961bf38b24f8f931dcf66799cbc468e473df
theStack:
ACK ff9d961bf38b24f8f931dcf66799cbc468e473df
Tree-SHA512: 592fabfab3218cec36c2d00a21cd535fa840daa126ee8440c384952fbb3913180aa3796066c630087e933d6517f19089b867f158e0b737f25283a14799eefb05
|
|
|
|
Co-authored-by: Jon Atack <jon@atack.com>
|
|
|
|
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.
|
|
ba616b932cb9e9adb7eb9f1826caa62ce422a22d wallet: Add GetPrefixCursor to DatabaseBatch (Andrew Chow)
1d858b055daeea363e0450f327672658548be4c6 walletdb: Handle when database keys are empty (Ryan Ofsky)
84b2f353bbefb9264284e7430863b2fa1d796d38 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 ba616b932cb9e9adb7eb9f1826caa62ce422a22d. Just suggested changes since last review
furszy:
ACK ba616b93
Tree-SHA512: 38a61849f108d8003d28c599b1ad0421ac9beb3afe14c02f1253e7b4efc3d4eef483e32647a820fc6636bca3f9efeff9fe062b6b602e0cded69f21f8b26af544
|
|
ParseNonRFCJSONValue() and rename it
cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687 refactor: rpc: hide and rename ParseNonRFCJSONValue() (stickies-v)
6c8bde6d54d03224709dce54b8ba32b8c3e37ac7 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 cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687. Only change since last review is adding a new test
Tree-SHA512: 89be959d2353af7ace0c1348ba1600b9ac1d3c7b3cf7f0b59f6e005b7fb9d695ce3e8720e1be3cf77fe7e318a4017c880df108928e7179ec50447583d13bc781
|
|
5d718f6913219d3ebe8394a17ddee81915e6f0ac 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 5d718f6913219d3ebe8394a17ddee81915e6f0ac
Tree-SHA512: f3c7cd2ef6716332136c75b43f6d54ce920be6f546a11bbf92b1fd65575607c42cc24b319691d86d0db038335636ba12b6387383a184f1589a8d71d1180f194f
|
|
method and << operator
5cd0717a54ce7a2065b29d90717aa2eb1c5e302d 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 5cd0717a54ce7a2065b29d90717aa2eb1c5e302d 🌙
Tree-SHA512: 49dd117de266f091a5336b13a91c5d8658abe1b3a0a9c51c8b5f6a2e0e814781b73afc39256353e79dade603a8a2761e8536716d1a48499720c266f4500477e2
|
|
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
|
|
2cd28e9fef5dd743bcd70025196ee311fcfdcae4 rpc: Add check for unintended option/parameter name clashes (Ryan Ofsky)
95d7de0964620a3f7386a4adc5707559868abf84 test: Update python tests to use named parameters instead of options objects (Ryan Ofsky)
96233146dd31c1d99fd1619be4449944623ef750 RPC: Allow RPC methods accepting options to take named parameters (Ryan Ofsky)
702b56d2a8ce48bc3b66a2867d09fa11dcf12fc5 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 2cd28e9fef5dd743bcd70025196ee311fcfdcae4
Tree-SHA512: 50f6e78fa622826dab3f810400d8c1a03a98a090b1f2fea79729c58ad8cff955554bd44c2a5975f62a526b900dda68981862fd7d7d05c17f94f5b5d847317436
|