Age | Commit message (Collapse) | Author |
|
The crash would happen when querying a mempool transaction with verbosity=2, while pruning.
Github-Pull: #29003
Rebased-From: 494a926d05df44b60b3bc1145ad2a64acf96f61b
|
|
A single outbound slot is required, so if the first two slots
are taken by inbound in-flights, the node will reject additional
unless they are coming from outbound.
This means in the case where a fast sybil peer is attempting to
stall out a node, a single high bandwidth outbound peer can
mitigate the attack.
Github-Pull: #27626
Rebased-From: 03423f8bd12b95a06a4a9d8377e781625dd38aae
|
|
{create,load,unload,restore}wallet
7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55 test: fix importmulti/importdescriptors assertion (Jon Atack)
19d888ce407f44d90785c456a1a3e2a6870e9245 rpc: move WALLET_FLAG_CAVEATS to the compilation unit of its caller (Jon Atack)
01df011ca2bf46ee4c988b03a130eea6df692325 doc: release note for wallet RPCs "warning" field deprecation (Jon Atack)
9ea8b3739a863b0ad87593639476b3cd712ff0dc test: createwallet "warning" field deprecation test (Jon Atack)
645d7f75ac1b40e4ea88119b3711f89943d35d6c rpc: deprecate "warning" field in {create,load,unload,restore}wallet (Jon Atack)
2f4a926e95e0379397859c3ba1b5711be5f09925 test: add test coverage for "warnings" field in createwallet (Jon Atack)
4a1e479ca612056761e6247dd5b715dcd6824413 rpc: add "warnings" field to RPCs {create,load,unload,restore}wallet (Jon Atack)
079d8cdda8eeebe199fb6592fca2630c37662731 rpc: extract wallet "warnings" fields to a util helper (Jon Atack)
f73782a9032a462a71569e9424db9bf9eeababf3 doc: fix/improve warning helps in {create,load,unload,restore}wallet (Jon Atack)
Pull request description:
Based on discussion and concept ACKed in #27138, add a `warnings` field to RPCs createwallet, loadwallet, unloadwallet, and restorewallet as a JSON array of strings to replace the `warning` string field in these 4 RPCs. The idea is to more gracefully handle multiple warning messages and for consistency with other wallet RPCs. Then, deprecate the latter fields, which represent all the remaining RPC `warning` fields.
The first commit https://github.com/bitcoin/bitcoin/pull/27279/commits/f73782a9032a462a71569e9424db9bf9eeababf3 implements https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1474789198 as an alternative to #27138. One of those two could potentially be backported to our currently supported releases.
ACKs for top commit:
achow101:
ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
1440000bytes:
utACK https://github.com/bitcoin/bitcoin/pull/27279/commits/7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
vasild:
ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
pinheadmz:
re-ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
Tree-SHA512: 314e0a4c41fa383d95e2817bfacf359d449e460529d235c3eb902851e2f4eacbabe646d9a5a4beabc4964cdfabf6397ed8301366a58d344a2f787f83b75e9d64
|
|
This new "warnings" field is a JSON array of strings intended to replace the
"warning" string field in these four RPCs, to better handle returning multiple
warning messages and for consistency with other wallet RPCs.
|
|
|
|
00e9b97f37e0bdf4c647236838c10b68b7ad5be3 refactor: Move fs.* to util/fs.* (TheCharlatan)
106b46d9d25b5228ef009fbbe6f9a7ae35090d15 Add missing fs.h includes (TheCharlatan)
b202b3dd6393b415fa68e18dc49c9431dc6b58b2 Add missing cstddef include in assumptions.h (TheCharlatan)
18fb36367a28819bd5ab402344802796a1248979 refactor: Extract util/fs_helpers from util/system (Ben Woosley)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". This commit was originally authored by empact and is taken from its parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library (https://github.com/bitcoin/bitcoin/pull/25290, https://github.com/bitcoin/bitcoin/pull/25487, https://github.com/bitcoin/bitcoin/pull/25527, https://github.com/bitcoin/bitcoin/pull/25862, https://github.com/bitcoin/bitcoin/pull/26177, and https://github.com/bitcoin/bitcoin/pull/27125). The `ArgsManager` is defined in `system.h`. A similar pull request extracting functionality from `system.h` has been merged in https://github.com/bitcoin/bitcoin/pull/27238.
#### Changes
Next to providing better code organization, this PR removes some reliance of the tree of libbitcoinkernel header includes on `system.h` (and thus the `ArgsManager` definition) by moving filesystem related functions out of the `system.*` files.
There is already a pair of `fs.h` / `fs.cpp` in the top-level `src/` directory. They were not combined with the files introduced here, to keep the patch cleaner and more importantly because they are often included without the utility functions. The new files are therefore named `fs_helpers` and the existing `fs` files are moved into the util directory.
Further commits splitting more functionality out of `system.h` are still in #25152 and will be submitted in separate PRs once this PR has been processed.
ACKs for top commit:
hebasto:
ACK 00e9b97f37e0bdf4c647236838c10b68b7ad5be3
Tree-SHA512: 31422f148d14ba3c843b99b1550a6fd77c77f350905ca324f93d4f97b652246bc58fa9696c64d1201979cf88733e40be02d262739bb7d417cf22bf506fdb7666
|
|
https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html
|
|
https://clang.llvm.org/extra/clang-tidy/checks/performance/faster-string-find.html
|
|
fa18504d5767a40dc9827fb081633219bf251001 rpc: Add submit option to generateblock (MarcoFalke)
fab9a08e145dc5a1d9576bf062473f1095b56a16 refactor: Replace block_hash with block_out (MarcoFalke)
Pull request description:
When submit is turned off, a block can be generated and returned as hex, to be used for further tests. For example, it can be submitted on a different node, on a different interface (like p2p), or just never submitted and be used for other testing purposes.
ACKs for top commit:
instagibbs:
ACK fa18504d5767a40dc9827fb081633219bf251001
TheCharlatan:
tACK fa18504d5767a40dc9827fb081633219bf251001
Tree-SHA512: 1b2ab6b71bb7e155c6482d75f5373f4e77de6446cb16bc2dfd19e7a4075b3a6ad87d7ad7a049a9eed934cb71574acfd27202f54c8bb3b03fac869f2e95db7ee5
|
|
The fs.* files are already part of the libbitcoin_util library. With the
introduction of the fs_helpers.* it makes sense to move fs.* into the
util/ directory as well.
|
|
This is an extraction of filesystem related functions from util/system
into their own utility file.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving these functions out of system.h allows including them from a
separate source file without including the ArgsManager definitions from
system.h.
|
|
|
|
fe329dc936d1e02da406345e4223e11d1fa6fb38 test: Add test for getblockfrompeer on pruned nodes (Fabian Jahr)
cd761e6b2cfade038b8018886c334bf25a0bcbcf rpc: Add note on guarantees to getblockfrompeer (Fabian Jahr)
Pull request description:
These are additions to `getblockfrompeer` that I already [suggested on the original PR](https://github.com/bitcoin/bitcoin/pull/20295#pullrequestreview-817157738).
The two commits do the following:
1. Add a test for `getblockfrompeer` usage on pruned nodes. This is important because many use-cases for `getblockfrompeer` are in a context of a pruned node.
2. Add some information on how long the users of pruned nodes can expect the block to be available after they have used the RPC. I think the behavior is not very intuitive for users and I would not be surprised if users expect the block to be available indefinitely.
ACKs for top commit:
Sjors:
re-utACK fe329dc936d1e02da406345e4223e11d1fa6fb38
MarcoFalke:
review ACK fe329dc936d1e02da406345e4223e11d1fa6fb38 π
stratospher:
ACK fe329dc.
brunoerg:
re-ACK fe329dc936d1e02da406345e4223e11d1fa6fb38
Tree-SHA512: a686bd8955d9c3baf365db384e497d6ee1aa9ce2fdb0733fe6150f7e3d94bae19d55bc1b347f1c9f619e749e18b41a52b9f8c0aa2042dd311a968a4b5d251fac
|
|
|
|
|
|
getrawtransaction when verbose is 2
3e947d7117c97a3cc34cfa7e1f5515fa0192fbe7 doc: remove 'omitted...' doc for rpc getrawtransaction when verbose is 2 (dougEfish)
Pull request description:
Remove optional rpc doc for getrawtransaction when verbose is 2
ACKs for top commit:
stickies-v:
ACK 3e947d7117c97a3cc34cfa7e1f5515fa0192fbe7
Tree-SHA512: b9e970d6ef4a47ec7ca32f5ff1028cc901f1bfdc1571668208505d42f4160733530601b78e469de82a854d3b298a55a81d0a7916bc5db4a43ad6d6a299c55c9e
|
|
|
|
tr() and miniscript
e4ede64fe87b1619f65da5753088d93e6f481b05 Expand scantxoutset help text to cover tr() and miniscript (Greg Sanders)
Pull request description:
ACKs for top commit:
achow101:
ACK e4ede64fe87b1619f65da5753088d93e6f481b05
darosior:
webACK e4ede64fe87b1619f65da5753088d93e6f481b05
Tree-SHA512: 6b5d9e7fccc8242f4534861c1b438ec40fb03fbf5968c5a4af5ddbced73df6d666812053c2e12a1e0a34057f6f4fe11987c8c59d8cdfa48ca7ab7d2720b51ef9
|
|
|
|
string_view
545ff924ab6303ffabd91fdfc4f0a4962daf133c refactor: use string_view for RPC named argument values (stickies-v)
7727603e44f8f674e0fc8389e78047e2b56e6052 refactor: reduce unnecessary complexity in ParseNonRFCJSONValue (stickies-v)
1d02e599012721549d4c20b1b37fcc5ee7b961b6 test: add cases to JSON parsing (stickies-v)
Pull request description:
Inspired by MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/26506#discussion_r1036149426). Main purpose of this PR is to minimize copying (potentially large) RPC named arguments when calling `.substr()` by using `std::string_view` instead of `std::string`. Furthermore, cleans up the code by removing unnecessary complexity in `ParseNonRFCJSONValue()` (done first to avoid refactoring required to concatenate `string` and `string_view`), updates some naming and adds a few test cases. Should not introduce any behaviour change.
## Questions
- ~Was there actually any merit to `ParseNonRFCJSONValue()` surrounding the value with brackets and then parsing it as an array? I don't see it, and the new approach doesn't fail any tests. Still a bit suspicious about it though.~
- Cleared up by https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059
- If there are no objections to 7727603e44f8f674e0fc8389e78047e2b56e6052, I think we should follow up with a PR to rename `ParseNonRFCJSONValue()` to a local `Parse()` helper function (that throws if invalid), remove it from `client.h` and merge the test coverage we currently have on `ParseNonRFCJSONValue()` with the coverage we have on `UniValue::read()`.
ACKs for top commit:
ryanofsky:
Code review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c
MarcoFalke:
review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c π»
Tree-SHA512: b1c89fb010ac9c3054b023cac1acbba2a539a09cf39a7baffbd7f7571ee268d5a6d98701c7ac10d68a814526e8fd0fe96ac1d1fb072f272033e415b753f64a5c
|
|
Most of the code in util/system.cpp that was hardcoded to use the global
ArgsManager instance `gArgs` has been changed to work with explicit ArgsManager
instances (for example in https://github.com/bitcoin/bitcoin/pull/20092). But a
few hardcoded references to `gArgs` remain. This commit removes the last ones
so these functions aren't reading or writing global state.
|
|
which sets a maximum value for unspendable outputs.
7013da07fbcddb04abae9759767a9419ab90444c Add release note for PR#25943 (David Gumberg)
04f270b4358417fc2827b9f91717816062b1864e Add test for unspendable transactions and parameter 'maxburnamount' to sendrawtransaction. (David Gumberg)
Pull request description:
This PR adds a user configurable, zero by default parameter β `maxburnamount` β to `sendrawtransaction`. This PR makes bitcoin core reject transactions that contain unspendable outputs which exceed `maxburnamount`. closes #25899.
As a result of this PR, `sendrawtransaction` will by default block 3 kinds of transactions:
1. Those that begin with `OP_RETURN` - (datacarriers)
2. Those whose lengths exceed the script limit.
3. Those that contain invalid opcodes.
The user is able to configure a `maxburnamount` that will override this check and allow a user to send a potentially unspendable output into the mempool.
I see two legitimate use cases for this override:
1. Users that deliberately use `OP_RETURN` for datacarrier transactions that embed data into the blockchain.
2. Users that refuse to update, or are unable to update their bitcoin core client would be able to make use of new opcodes that their client doesn't know about.
ACKs for top commit:
glozow:
reACK 7013da07fbcddb04abae9759767a9419ab90444c
achow101:
re-ACK 7013da07fbcddb04abae9759767a9419ab90444c
Tree-SHA512: f786a796fb71a587d30313c96717fdf47e1106ab4ee0c16d713695e6c31ed6f6732dff6cbc91ca9841d66232166eb058f96028028e75c1507324426309ee4525
|
|
VerifyDB dosn't finish successfully
0af16e7134459e0820ab95d751093876c1ec4c6d doc: add release note for #25574 (Martin Zumsande)
57ef2a4812f443b2d734f43cebf3ef5038da83f2 validation: report if pruning prevents completion of verification (Martin Zumsande)
0c7785bb2540b69564104767d38342704230cbc2 init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache (Martin Zumsande)
d6f781f1cfcbc2c2ad5ee289a0642ed00386d013 validation: return VerifyDBResult::INTERRUPTED if verification was interrupted (Martin Zumsande)
6360b5302d2675788de5c4a28ea77d823f6d809e validation: Change return value of VerifyDB to enum type (Martin Zumsande)
Pull request description:
`VerifyDB()` can fail to complete due to insufficient dbcache at the level 3 checks. This PR improves the error handling in this case in the following ways:
- The rpc `-verifychain` now returns false if the check can't be completed due to insufficient cache
- During init, we only log a warning if the default values for `-checkblocks`Β and `-checklevel` are taken and the check doesn't complete. However, if the user actively specifies one of these args, we return with an InitError if we can't complete the check.
This PR also changes `-verifychain` RPC to return `false` if the verification didn't finish due to missing block data (pruning) or due to being interrupted by the node being shutdown.
Previously, this PR also included a fix for a possible assert during verification - this was done in #27009 (now merged).
ACKs for top commit:
achow101:
ACK 0af16e7134459e0820ab95d751093876c1ec4c6d
ryanofsky:
Code review ACK 0af16e7134459e0820ab95d751093876c1ec4c6d. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups
john-moffett:
ACK 0af16e7134459e0820ab95d751093876c1ec4c6d
MarcoFalke:
lgtm re-ACK 0af16e7134459e0820ab95d751093876c1ec4c6d π
Tree-SHA512: 84b4f767cf9bfbafef362312757c9bf765b41ae3977f4ece840e40c52a2266b1457832df0cdf70440be0aac2168d9b58fc817238630b0b6812f3836ca950bc0e
|
|
sendrawtransaction.
'maxburnamount' sets a maximum value for outputs heuristically deemed unspendable including datacarrier scripts that begin with `OP_RETURN`.
|
|
`submitpackage` error msg
7554b1fd663fe2010edb0e8a93ab85a6cb10a323 rpc: fix successful broadcast count in `submitpackage` error msg (Sebastian Falbesoner)
Pull request description:
If a `submitpackage` RPC call errors due to any of the individual tx broadcasts failing, the returned error message is supposed to contain the number of successful broadcasts so far:
https://github.com/bitcoin/bitcoin/blob/4395b7f0845d2dca60f3b4e007ef5770ce8e2aa9/src/rpc/mempool.cpp#L848-L849
Right now this is wrongly always shown as zero. Fix this by adding the missing increment of the counter. While touching that area, the variable is also renamed to better reflect its purpose (s/num_submitted/num_broadcast/; the submission has already happened at that point) and named arguments for the `BroadcastTransaction` call are added.
(Note that the error should be really rare, as all txs have already been submitted succesfully to the mempool. IIUC this code-path could only hit if somehow a tx is being removed from the mempool between `ProcessNewPackage` and the `BroadcastTransaction` calls, e.g. if a new block is received which confirms any of the package's txs.)
ACKs for top commit:
glozow:
utACK 7554b1fd663fe2010edb0e8a93ab85a6cb10a323, thanks!
Tree-SHA512: e362e93b443109888e28d6facf6f52e67928e8baaa936e355bfdd324074302c4832e2fa0bd8745309a45eb729866d0513b928ac618ccc9432b7befc3aa2aac66
|
|
to allow inferring descriptors for scripts larger than 520 bytes
73ec4b2a8347c796b9aadc1f2576b286c469f9e7 tests: decodescript can infer descriptors for scripts >520 bytes (Andrew Chow)
7cc78223710679c6e7fd40b762798a1f5ca4938e rpc: Use FlatSigningProvider in decodescript (Andrew Chow)
Pull request description:
`FillableSigningProvider` limits scripts to 520 bytes even though segwit allows scripts to be larger than that. We can avoid this limit by using a `FlatSigningProvider` so that such larger scripts can be decoded.
Fixes #27111
ACKs for top commit:
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/27113/commits/73ec4b2a8347c796b9aadc1f2576b286c469f9e7
Tree-SHA512: c0e6d21025e2da864471989ac94c54e127d05459b9b048f34a0da8d76d8e372d5472a2e667ba2db74d6286e3e6faa55486ffa9232a068b519afa676394031d5a
|
|
If a `submitpackage` RPC call errors due to any of the individual tx
broadcasts failing, the returned error message is supposed to contain
the number of successful broadcasts so far. Right now this is wrongly
always shown as zero. Fix this by adding the missing counting.
(Note though that the error should be really rare, as all txs have
already been submitted succesfully to the mempool.)
|
|
CService and use better naming
c9d548c91fb12fba516dee896f1f97692cfa2104 net: remove CService::ToStringPort() (Vasil Dimov)
fd4f0f41e915d99c9b0eac1afd21c5628222e368 gui: simplify OptionsDialog::updateDefaultProxyNets() (Vasil Dimov)
96c791dd20fea54c17d224000dee677bc158f66a net: remove CService::ToString() use ToStringAddrPort() instead (Vasil Dimov)
944a9de08a00f8273e73cd28b40e46cc0eb0bad1 net: remove CNetAddr::ToString() and use ToStringAddr() instead (Vasil Dimov)
043b9de59aec88ae5e29daac7dc2a8b51a9414ce scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]() (Vasil Dimov)
Pull request description:
Before this PR we had the somewhat confusing combination of methods:
`CNetAddr::ToStringIP()`
`CNetAddr::ToString()` (duplicate of the above)
`CService::ToStringIPPort()`
`CService::ToString()` (duplicate of the above, overrides a non-virtual method from `CNetAddr`)
`CService::ToStringPort()`
Avoid [overriding non-virtual methods](https://github.com/bitcoin/bitcoin/pull/25349/#issuecomment-1185226396).
"IP" stands for "Internet Protocol" and while sometimes "IP addresses" are called just "IPs", it is incorrect to call Tor or I2P addresses "IPs". Thus use "Addr" instead of "IP".
Change the above to:
`CNetAddr::ToStringAddr()`
`CService::ToStringAddrPort()`
The changes touch a lot of files, but are mostly mechanical.
ACKs for top commit:
sipa:
utACK c9d548c91fb12fba516dee896f1f97692cfa2104
achow101:
ACK c9d548c91fb12fba516dee896f1f97692cfa2104
jonatack:
re-ACK c9d548c91fb12fba516dee896f1f97692cfa2104 only change since my previous reviews is rebase, but as a sanity check rebased to current master and at each commit quickly re-reviewed and re-verified clean build and green unit tests
LarryRuane:
ACK c9d548c91fb12fba516dee896f1f97692cfa2104
Tree-SHA512: 633fb044bdecf9f551b5e3314c385bf10e2b78e8027dc51ec324b66b018da35e5b01f3fbe6295bbc455ea1bcd1a3629de1918d28de510693afaf6a52693f2157
|
|
dbcache
The rpc command verifychain now fails if the dbcache was not sufficient
to complete the verification at the specified level and depth.
In the same situation, the VerifyDB check during Init will now fail (and lead to
an early shutdown) if the user has explicitly specified -checkblocks or
-checklevel but the check couldn't be executed because of the limited
cache. If the user didn't change any of the two and is using the defaults, log a warning
but don't prevent the node from starting up.
|
|
This does not change behavior. It is in preparation for
special handling of the case where VerifyDB doesn't finish
for various reasons, but doesn't fail.
|
|
4c8ecccdcd813fac3a7ef6a1493ef3977220421d test: add tests for `outputs` argument to `bumpfee`/`psbtbumpfee` (Seibart Nedor)
c0ebb9838252fb187db8719755801758d89651f7 wallet: add `outputs` arguments to `bumpfee` and `psbtbumpfee` (Seibart Nedor)
a804f3cfc0b4761b9ca7976e6e4472cd93599bbf wallet: extract and reuse RPC argument format definition for outputs (Seibart Nedor)
Pull request description:
This implements a modification of the proposal in #22007: instead of **adding** outputs to the set of outputs in the original transaction, the outputs given by `outputs` argument **completely replace** the outputs in the original transaction.
As noted below, this makes it easier to "cancel" a transaction or to reduce the amounts in the outputs, which is not the case with the original proposal in #22007, but it seems from the discussion in this PR that the **replace** behavior is more desirable than **add** one.
ACKs for top commit:
achow101:
ACK 4c8ecccdcd813fac3a7ef6a1493ef3977220421d
1440000bytes:
Code Review ACK https://github.com/bitcoin/bitcoin/pull/25344/commits/4c8ecccdcd813fac3a7ef6a1493ef3977220421d
ishaanam:
reACK 4c8ecccdcd813fac3a7ef6a1493ef3977220421d
Tree-SHA512: 31361f4a9b79c162bda7929583b0a3fd200e09f4c1a5378b12007576d6b14e02e9e4f0bab8aa209f08f75ac25a1f4805ad16ebff4a0334b07ad2378cc0090103
|
|
Using a FillableSigningProvider results in decodescript being unable to
infer descriptors for scripts larger than 520 bytes. Using a
FlatSigningProvider resolves this.
|
|
it adds `Ensure{any}Banman` functions to avoid
code repetition and make it cleaner. Similar
approach as done with argsman, chainman, connman
and others.
|
|
This change makes all filesystem artifacts--files and directories--being
created with the default umask.
|
|
This change effectively reverts commits from
https://github.com/bitcoin/bitcoin/pull/4286.
Users, who rely on non-default access permissions, should use `chmod`
command.
|
|
The descriptor inference logic would previously always use a dummy
signing provider and would never analyze the witness script of a P2WSH
scriptPubKey.
Note even a valid Miniscript might not always be decodable from Script
without more contextual information (for instance the key preimage for a
pk_h).
|
|
to SHOW_DETAILS
a24e633339c45eaca28fc7af0488956332ac300c refactor: rpc: set TxToJSON default verbosity to SHOW_DETAILS (stickies-v)
Pull request description:
`TxToJSON()` and `TxToUniv()` are only to be called when we want to decode the transaction (i.e. its details) into JSON. If `TxVerbosity` is `SHOW_TXID`, the function should not have been (and currently is not) called in the first place.
There is no behaviour change, current logic simply assumes anything less than `TxVerbosity::SHOW_DETAILS_AND_PREVOUT` equals `TxVerbosity::SHOW_DETAILS`. With this change, the assumptions and intent become more explicit.
ACKs for top commit:
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/26974/commits/a24e633339c45eaca28fc7af0488956332ac300c
Tree-SHA512: b97235adae49b972bdbe10aca1438643fb35ec66a4e57166b1975b3015bc5a06a711feebe4453a8fefe71781e484b21ef80847d8e8a33694a3abcc863accd4d7
|
|
faf7b4f1fc35f1488567e0e4a57ecb348596b992 Add BlockManager::IsPruneMode() (MarcoFalke)
fae71fe27ec021583aaeac09aa924522bb63db05 Add BlockManager::GetPruneTarget() (MarcoFalke)
fa0f0436d83288262d7d764b1d239c1a6de6146f Add BlockManager::LoadingBlocks() (MarcoFalke)
Pull request description:
Requested in https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1061323795, but adding getters seems unrelated from removing globals, so I split it out for now.
ACKs for top commit:
dergoegge:
Code review ACK faf7b4f1fc35f1488567e0e4a57ecb348596b992
brunoerg:
crACK faf7b4f1fc35f1488567e0e4a57ecb348596b992
Tree-SHA512: 204d0e9a0e8b78175482f89b4ce620fba0e65d8e49ad845d187af44d3843f4c733a01bac1ffe5a5319f524d8346123693a456778b69d6c75268c447eb8839642
|
|
`TxToJSON()` and `TxToUniv()` are only to be called when we want to
decode the transaction (i.e. its details) into JSON. If `TxVerbosity` is
`SHOW_TXID`, the function should not have been (and currently is not)
called in the first place.
There is no behaviour change, current logic simply assumes anything less
than `TxVerbosity::SHOW_DETAILS_AND_PREVOUT` equals
`TxVerbosity::SHOW_DETAILS`. With this change, the assumptions and
intent become more explicit.
|
|
and use it where possible
fa035fe2d61d0c98d1bfd0153a0c3b5eb9d40de4 Remove unused CDataStream::SetType (MarcoFalke)
fa29e73cdab82f98682821322cda89b1084ba887 Use DataStream where possible (MarcoFalke)
fa9becfe1cea5040e7cea36324d1b0789cbbd25d streams: Add DataStream without ser-type and ser-version (MarcoFalke)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `DataStream`. `CDataStream` remains in places where it is not yet possible.
ACKs for top commit:
stickies-v:
re-ACK [fa035fe](https://github.com/bitcoin/bitcoin/commit/fa035fe2d61d0c98d1bfd0153a0c3b5eb9d40de4)
aureleoules:
diff re-ACK fa035fe2d61d0c98d1bfd0153a0c3b5eb9d40de4 https://github.com/bitcoin/bitcoin/compare/fa0e6640bac8c6426af7c5744125c85c0f74b9e5..fa035fe2d61d0c98d1bfd0153a0c3b5eb9d40de4
Tree-SHA512: cb5e53d0df7c94319ffadc6ea1d887fc38516decaf43f0673396d79cc62d450a1a61173654a91b8c2b52d2cecea53fe4a500b8f6466596f35731471163fb051c
|
|
|
|
error (1.5/2)
fafeddfe0e6748e9769ad3dd526a6c0eaf6f4aac rpc: Throw more user friendly arg type check error (MarcoFalke)
Pull request description:
The arg type check error doesn't list which arg (position or name) failed. Fix that.
ACKs for top commit:
stickies-v:
ACK fafeddfe0e6748e9769ad3dd526a6c0eaf6f4aac - although I think the functional test isn't in a logical place (but not blocking)
Tree-SHA512: 17425aa145aab5045940ec74fff28f0e3b2b17ae55f91c4bb5cbcdff0ef13732f8e31621d85964dc2c04333ea37dbe528296ac61be27541384b44e37957555c8
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i -e "/Deprecated alias for OMITTED, can be removed/d" src/rpc/util.h src/rpc/util.cpp
sed -i -e "s/OMITTED_NAMED_ARG/OMITTED/g" $(git grep -l "OMITTED_NAMED_ARG" src/)
-END VERIFY SCRIPT-
|
|
|
|
errors
3d1a4d8a45cd91bdfe0ef107c2e9c5e882b34155 RPC: make RPCResult::MatchesType return useful errors (Anthony Towns)
Pull request description:
Currently if you don't correctly update the description of the return value for an RPC call, you essentially just get an assertion failure with no useful information; this generates a description of the problems instead.
ACKs for top commit:
MarcoFalke:
re-ACK 3d1a4d8a45cd91bdfe0ef107c2e9c5e882b34155 π·
Tree-SHA512: cf0580b7046faab0128672a74f8cc5a1655dfdca6646a2e38b51f0fb5f672c98aad6cd4c5769454a2d644a67da639ccb1c8ff5d24d3d6b4446a082398a643722
|
|
|
|
Minimize copying RPC named argument values when calling .substr() by
using std::string_view instead of std::string.
|
|
Since https://github.com/jgarzik/univalue/pull/31, UniValue::read() can now
parse raw literals directly, so there is no more need to wrap them into an
array first.
|
|
fad56f7dd6ecac772e1bf590b38cd34ba67db5d7 doc: Properly report optional RPC args (MarcoFalke)
fa09cb60865a20cc3bdc84b4001fa503f1061319 refactor: Introduce is_top_level_arg (MarcoFalke)
fab92a5a5ab45227dbb0aa6d2cf4557b157b17e6 refactor: Remove const to fix performance-move-const-arg clang-tidy errors (MarcoFalke)
Pull request description:
`OMITTED_NAMED_ARG` and `OMITTED` are a confusing burden:
* It puts the burden on developers to pick the right one of the two
* They can be interchanged without introducing a compile failure or other error
* Picking the wrong one is leading to incorrect docs
* They are redundant, because the correct one can already be determined by the surrounding type
Fix all issues by making them an alias of each other; Pick the right one based on the outer type.
ACKs for top commit:
fanquake:
ACK fad56f7dd6ecac772e1bf590b38cd34ba67db5d7
Tree-SHA512: 6e7193a05a852ba1618a9cb3261220c7ad3282bc5595325c04437aa811f529a88e2851e9c7dbf9878389b8aa42e98f8817b7eb0260fbb9e123da0893cbae6ca2
|