Age | Commit message (Collapse) | Author |
|
daed542a12e0a6a4692aca12a61b84cd55accc33 [net_processing] Move ProcessMessage to PeerLogicValidation (John Newbery)
c556770b5ef4306f10e980daff772bee87daaa30 [net_processing] Change PeerLogicValidation to hold a connman reference (John Newbery)
Pull request description:
Rather than ProcessMessage() being a static function in net_processing.cpp, make it a private member function of PeerLogicValidation. This is the start of moving static functions and global variables into PeerLogicValidation to make it better encapsulated.
ACKs for top commit:
jonatack:
ACK daed542a12e0a6a4692aca12a61b84cd55accc33 code review and debug tested
promag:
Code review ACK daed542a12e0a6a4692aca12a61b84cd55accc33.
MarcoFalke:
re-ACK daed542a12, only change is removing second commit 🎴
theStack:
Code Review ACK daed542a12e0a6a4692aca12a61b84cd55accc33
Tree-SHA512: ddebf410d114d9ad5a9e536950018ff333a347c035d74fcc101fb4a3f20a281782c7eac2b7d1bd1c8f6bc7e59f5b5630fb52c2e1b4c32df454fa584673bd021e
|
|
It does count the coinbase transaction.
Refs #19766
|
|
|
|
8ed2f1ed78937eff0bb8b5318a30da908e33af24 Remove unused includes (Marcin Jachymiak)
cf095a53fcef8ad72e2f1177660ef50bc7e340ad Move comment about BaseIndex::DB from TxIndex::DB (Marcin Jachymiak)
Pull request description:
Moves a comment about the `BaseIndex::DB` from the `TxIndex::DB` into the correct place. Originally part of https://github.com/bitcoin/bitcoin/pull/14053.
ACKs for top commit:
fanquake:
ACK 8ed2f1ed78937eff0bb8b5318a30da908e33af24
Tree-SHA512: cb4e2b916c7ab996961cc2e1d910bc4b8a1700eb32b70fc1657ca720117a7a84f7337fe5e4fb30e047aa92c31eaa976eaaa5cb8f861877f2ff6f4a59bb94f4e9
|
|
124e1ee1343f8bfb3748393ced9debdbdee60d3b doc: Add release notes for getindexinfo RPC (Fabian Jahr)
c447b09458c89c946957a211a4f5373b92af44bf test: Add tests for getindexinfo RPC (Fabian Jahr)
667bc7a7f7c5d9a15eaf6957c3d8841a75efa7bc rpc: Add getindexinfo RPC (Fabian Jahr)
Pull request description:
As I was playing with indices a I was missing an RPC that gives information about the active indices in the node. I think this can be helpful for many users, especially since there are some new index candidates coming up (#14053, #18000) that can give a quick overview without the user having to parse the logs.
Feature summary:
- Adds new RPC `listindices` (placed in Util section)
- That RPC only lists the actively running indices
- For each index it gives the name, whether it is synced and up to which block height it is synced
ACKs for top commit:
laanwj:
Re-ACK 124e1ee1343f8bfb3748393ced9debdbdee60d3b
jonatack:
Code review re-ACK 124e1ee per `git range-diff a57af89 47a5372 124e1ee` no change since my last re-ACK, rebase only
Tree-SHA512: 3b7174c87951e6457fef099f530337803906baf32fb64261410b8def2c0917853d6a1bf3059cd590b1cc1523608f8916dafb327a431d27ecbf8d7454406b5b35
|
|
333329dbda423b00098ec9f8702d75d24468c56e doc: Fix getmempoolancestor RPC result doc (MarcoFalke)
Pull request description:
ACKs for top commit:
laanwj:
ACK 333329dbda423b00098ec9f8702d75d24468c56e
Tree-SHA512: 30a7568ec15d1af0c484b4d479e14ec3609a01b76f17f8285688b0c5e5b0480926bbf6f651da91193b66fd752e83e9707e4b08c52b69f8c670c430da84713359
|
|
71e0f07e9c5f0aef532b85c04807dcbedd04e0af util: remove unused c-string variant of atoi64() (Sebastian Falbesoner)
Pull request description:
This is another micro-PR "removing old cruft with potentially sharp edges" (quote by practicalswift, see #19739). Gets rid of the c-string variant of the function `atoi64()`, which is only used in fuzzers and on one place with `wallet/wallet.h` (where it is originally a `std::string` anyways and uses `.c_str()` -- this method call can simply be removed.)
ACKs for top commit:
practicalswift:
ACK 71e0f07e9c5f0aef532b85c04807dcbedd04e0af -- diff looks correct
laanwj:
ACK 71e0f07e9c5f0aef532b85c04807dcbedd04e0af
Tree-SHA512: 4d1d28e2f5274fdbe0652e7a0f83dd416f4d19c1e1a49979927960a3ad40b0990eeaa4374656bf2c6998a965a14d62c1bc78303b7d583d3307c17828030a8e3b
|
|
356988e200b1debaa80d210d502d2d085c72dc64 util: make EncodeBase58Check consume Spans (Sebastian Falbesoner)
f0fce0675d56b2226a993253731690ca864066c8 util: make EncodeBase58 consume Spans (Sebastian Falbesoner)
Pull request description:
This PR improves the interfaces for the functions `EncodeBase58{Check}` by using Spans, in a similar fashion to e.g. PRs #19660, #19687. Note that on the master branch there are currently two versions of `EncodeBase58`: one that takes two pointers (marking begin and end) and another one that takes a `std::vector<unsigned char>` const-ref. The PR branch only leaves one generic Span-interface, both simplifying the interface and allowing more generic containers to be passed. The same is done for `EncodeBase58Check`, where only one interface existed but it's more generic now (e.g. a std::array can be directly passed, as done in the benchmarks).
ACKs for top commit:
laanwj:
Code review ACK 356988e200b1debaa80d210d502d2d085c72dc64
Tree-SHA512: 47cfccdd7f3a2d4694bb8785e6e5fd756daee04ce1652ee59a7822e7e833b4a441ae9362b9bd67ea020d2b5b7d927629c9addb6abaa9881d8564fd3b1257f512
|
|
|
|
|
|
|
|
Co-authored-by: Jon Atack <jon@atack.com>
Co-authored-by: Samuel Dobson <dobsonsa68@gmail.com>
|
|
4792cad88c5c3c93e639a051df779230ee817396 doc: comment out and add annotation to unused MSG_FILTERED_WITNESS_BLOCK (Adam Jonas)
Pull request description:
Commenting out and adding a note to unused `MSG_FILTERED_WITNESS_BLOCK` [defined in BIP144](https://github.com/bitcoin/bips/blob/master/bip-0144.mediawiki#relay).
There was an attempt to make use of this in https://github.com/bitcoin/bitcoin/pull/10350, but it was closed due to lack of support. (h/t sdaftuar for pointing to the PR and jnewbery for the idea)
ACKs for top commit:
jnewbery:
Obvious ACK 4792cad88c5c3c93e639a051df779230ee817396
theStack:
ACK https://github.com/bitcoin/bitcoin/pull/19721/commits/4792cad88c5c3c93e639a051df779230ee817396 📜
MarcoFalke:
cr ACK 4792cad88c5c3c93e639a051df779230ee817396 good to keep it around in a comment to avoid accidental future re-assignment
practicalswift:
ACK 4792cad88c5c3c93e639a051df779230ee817396
Tree-SHA512: 22327ddded643ae50fdb529e4529a9b464f74e90620d0d2079a11070eaa8afe8363f6e14cca52f3bec2c9f87ee13e318edc6c5193761c94b8ae77be353a8da1f
|
|
fa55c1d5fdd88c4bc4d361da231cd63b20255b50 build: Add Werror=range-loop-analysis (MarcoFalke)
Pull request description:
The warning is implicitly enabled for Bitcoin Core. Also explicitly since commit d92204c900d.
To avoid "fix range loop" follow-up refactors, we have two options:
* Disable the warning, so that issues never appear
* Enable it as an error, so that issues are either caught locally or by ci
ACKs for top commit:
fanquake:
ACK fa55c1d5fdd88c4bc4d361da231cd63b20255b50
practicalswift:
ACK fa55c1d5fdd88c4bc4d361da231cd63b20255b50 -- pre-review fix-up is better than post-review fix-up
hebasto:
re-ACK fa55c1d5fdd88c4bc4d361da231cd63b20255b50
Tree-SHA512: 019aa133f254af8882c1d5d10c420d9882305db0fc2aa9dad7d285168e2556306c3eedcc03bd30e63f11eae4cc82b648d83fb6e9179d6a6364651fb602d70134
|
|
|
|
7668db3b08531a590089d66cc5c91f1fb3afbfcc Move only: Move CDiskTxPos to its own file (Marcin Jachymiak)
Pull request description:
Moves `CDiskTxPos` it its own file so it can be used without the `txindex.h` include elsewhere. Originally part of #14053.
ACKs for top commit:
jnewbery:
utACK 7668db3b08531a590089d66cc5c91f1fb3afbfcc
promag:
ACK 7668db3b08531a590089d66cc5c91f1fb3afbfcc.
Tree-SHA512: b108e980ad04e43d1323410c3683a82bed70aee7795f5d8a2afbaf32a07ba598571f00b047bdde15048124b17178bcbd10654c48461beac988e9643cb2df664c
|
|
specified range
7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9 test: test the implicit avoid partial spends functionality (Karl-Johan Alm)
b82067bf696c53f22536f9ca2e51949c164f6b06 wallet: try -avoidpartialspends mode and use its result if fees are below threshold (Karl-Johan Alm)
Pull request description:
The `-avoidpartialspends` feature is normally disabled, as it may affect the optimal fee for payments. This PR introduces a new parameter `-maxapsfee` (max avoid partial spends fee) which acts on the following values:
* -1: disable partial spend avoidance completely (do not even try it)
* 0: only do partial spend avoidance if fees are the same or better as the regular coin selection
* 1..∞: use APS variant if the absolute fee difference is less than or equal to the max APS fee
For values other than -1, the code will now try partial spend avoidance once, and if that gives a value within the accepted range, it will use that.
Example: -maxapsfee=0.00001000 means the wallet will do regular coin select, APS coin select, and then pick AKS iff the absolute fee difference is <= 1000 satoshi.
Edit: updated this to reflect the fact we are now using a max fee.
ACKs for top commit:
fjahr:
tested ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9
achow101:
ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9
jonatack:
ACK 7f13dfb58, code review, debug build, verified the test fails with `AssertionError: not(2 == 1)` for the number of vouts when `-maxapsfee=0.0001` is changed to 0, and verified the new logging with an added assertion.
meshcollider:
Code review ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9
Tree-SHA512: 475929df57f6191bb4e36bfbcad5a280a64bb0ecd8767b76cb2e44e2301235d0eb294a3f2fac5bbf15d35d7ecfba47acb2285feadb883c9ce31c08377e3afb3c
|
|
767073fb9645f5cb0976a14288c03fe71912b569 Shrink CAddress from 48 to 40 bytes on x64 (Vasil Dimov)
Pull request description:
`CAddress` inherits `CService` which is 28 bytes (on 64 bit machines).
`CAddress` then adds two member variables - one that requires 4 byte
alignment (`nTime`) and one that requires 8 byte alignment
(`nServices`).
Declare the smaller one first so that it fits in bytes 29..32.
On 32 bit machines this change has no effect and `CAddress` remains 40
bytes.
ACKs for top commit:
laanwj:
ACK 767073fb9645f5cb0976a14288c03fe71912b569
theStack:
ACK https://github.com/bitcoin/bitcoin/commit/767073fb9645f5cb0976a14288c03fe71912b569
Tree-SHA512: 73d6a4fcfa2687b4076950801871252e369510ecf09f820576dbeca9ee3ee94d14672e7d5596cb45fedd9e4b973dd0716a2ea3f13fc3058b4b697d036a7c9db0
|
|
|
|
|
|
6cb8771173d835bdeb89c76b1d53191d3c896968 build: Add missed gcov files to 'make clean' (Hennadii Stepanov)
Pull request description:
On master (b4d0366b47dd9b8fe29cc9a100dcdf6ca1d3cabf):
```
$ ./autogen.sh
$ ./configure --enable-lcov
$ make && make cov
$ make clean
$ find . -name '*.gcno'
./src/rpc/libbitcoin_server_a-blockchain.gcno
./src/rpc/libbitcoin_common_a-util.gcno
./src/rpc/libbitcoin_common_a-rawtransaction_util.gcno
./src/rpc/libbitcoin_server_a-rawtransaction.gcno
./src/rpc/libbitcoin_util_a-request.gcno
./src/rpc/libbitcoin_server_a-net.gcno
./src/rpc/libbitcoin_server_a-server.gcno
./src/rpc/libbitcoin_server_a-mining.gcno
./src/rpc/libbitcoin_server_a-misc.gcno
./src/rpc/libbitcoin_cli_a-client.gcno
./src/node/libbitcoin_server_a-coinstats.gcno
./src/node/libbitcoin_server_a-transaction.gcno
./src/node/libbitcoin_server_a-context.gcno
./src/node/libbitcoin_server_a-psbt.gcno
./src/node/libbitcoin_server_a-ui_interface.gcno
./src/node/libbitcoin_server_a-coin.gcno
./src/test/util/libtest_util_a-setup_common.gcno
./src/test/util/libtest_util_a-net.gcno
./src/test/util/libtest_util_a-blockfilter.gcno
./src/test/util/libtest_util_a-mining.gcno
./src/test/util/libtest_util_a-transaction_utils.gcno
./src/test/util/libtest_util_a-wallet.gcno
./src/test/util/libtest_util_a-str.gcno
./src/test/util/libtest_util_a-logging.gcno
./src/index/libbitcoin_server_a-txindex.gcno
./src/index/libbitcoin_server_a-base.gcno
./src/index/libbitcoin_server_a-blockfilterindex.gcno
./src/util/libbitcoin_util_a-error.gcno
./src/util/libbitcoin_util_a-rbf.gcno
./src/util/libbitcoin_util_a-message.gcno
./src/util/libbitcoin_util_a-time.gcno
./src/util/libbitcoin_util_a-moneystr.gcno
./src/util/libbitcoin_util_a-url.gcno
./src/util/libbitcoin_consensus_a-strencodings.gcno
./src/util/libbitcoin_util_a-settings.gcno
./src/util/libbitcoin_util_a-system.gcno
./src/util/libbitcoin_util_a-threadnames.gcno
./src/util/libbitcoin_util_a-fees.gcno
./src/util/libbitcoin_util_a-asmap.gcno
./src/util/libbitcoin_util_a-strencodings.gcno
./src/util/libbitcoin_util_a-string.gcno
./src/util/libbitcoin_util_a-bytevectorhash.gcno
./src/util/libbitcoin_util_a-bip32.gcno
./src/util/libbitcoin_util_a-spanparsing.gcno
./src/util/libbitcoinconsensus_la-strencodings.gcno
./src/interfaces/libbitcoin_wallet_a-wallet.gcno
./src/interfaces/libbitcoin_util_a-handler.gcno
./src/interfaces/libbitcoin_server_a-chain.gcno
./src/interfaces/libbitcoin_server_a-node.gcno
./src/crc32c/src/libcrc32c_a-crc32c_portable.gcno
./src/crc32c/src/libcrc32c_a-crc32c.gcno
./src/crc32c/src/libcrc32c_sse42_a-crc32c_sse42.gcno
```
This PR fixes this issue.
ACKs for top commit:
practicalswift:
ACK 6cb8771173d835bdeb89c76b1d53191d3c896968 -- patch looks correct
Tree-SHA512: d331b8fa18f2e0cb2c107de747a39a018f73bcc20b05ed403aa38cf316b5be30b3229e92fb6c85469747571d0048a34b2846432994d0911c8234d207d4e32f1a
|
|
7966aa424a8b78983f73742cbdb3d11eccaf9f3a Add variables for repeated scripts (MeshCollider)
fec8336ad97dc717ea123f84ecfc10d9ee4a11db Remove GetScriptForWitness function (MeshCollider)
b887060d06290abf4983a487f8da6b0986b058ab Replace usage of GetScriptForWitness with GetScriptForDestination (MeshCollider)
Pull request description:
As per this TODO in the code:
> TODO: replace calls to GetScriptForWitness with GetScriptForDestination using the various witness-specific CTxDestination subtypes.
The commit "Add additional check for P2SH before adding extra wrapper" also adds an additional check that the scriptPubKey is a P2SH before auto-wrapping the witness script. We shouldn't wrap the witness script if not. Note: #16251 is even better than this check, please review that.
ACKs for top commit:
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/16841/commits/7966aa424a8b78983f73742cbdb3d11eccaf9f3a
jonatack:
Code review re-ACK 7966aa4 per `git range-diff b4d0366 ed266f7 7966aa4`
achow101:
re-ACK 7966aa424a8b78983f73742cbdb3d11eccaf9f3a only changes since last is rebase.
Tree-SHA512: 3449e0e83bd842acc7c94544a85367da97ac20d859eefc1a618caef0c98204398c266fe8fb9600b78326df5175402e1ae4a132eb766e2c4485e7cda6a2a95c43
|
|
642ad31b418bbf8da06cb3641329b0810e18e55b Add loadwallet and createwallet RPC load_on_startup options (Russell Yanofsky)
Pull request description:
This maintains a persistent list of wallets stored in settings that will automatically be loaded on startup. Being able to load a wallet automatically on startup will be more useful in the GUI, but it's reasonable to expose this feature by RPC as well.
ACKs for top commit:
achow101:
re-ACK 642ad31b418bbf8da06cb3641329b0810e18e55b Only change is the test
meshcollider:
re-utACK 642ad31b418bbf8da06cb3641329b0810e18e55b
Tree-SHA512: cca0b71bf1a83ad071830e6c459f1cd979b4add7144e899ec560da72b5910dd9bf9426e5c7d125ae96fad8990fbf81a76bc83c0459486c16086ada6cbde5eaa3
|
|
filtering to occur within the struct
9adc2f80fc14f11ee2b1f989ee7be71b58481e6f Refactor OutputGroups to handle effective values, fees, and filtering (Andrew Chow)
7d07e864b8846be186648814a5aaf34269f914a3 Use real value when calculating OutputGroup value (Andrew Chow)
Pull request description:
Currently, the effective values and filtering for positive effective values is done outside of the OutputGroup. We should instead have functions in Outputgroup to do this and call those for each OutputGroup. So this PR does that.
This makes future changes for effective values in coin selection much easier.
ACKs for top commit:
instagibbs:
reACK 9adc2f80fc14f11ee2b1f989ee7be71b58481e6f
fjahr:
re-ACK 9adc2f80fc14f11ee2b1f989ee7be71b58481e6f
meshcollider:
Light code review ACK 9adc2f80fc14f11ee2b1f989ee7be71b58481e6f
Tree-SHA512: 7445c94b7295b45bcd83a6f8a5c8f6961a89453fcc856335192d4b4a66aec7724513616b04e5111588ab208c89b311055399d6279cd9c4ce452aefb85f04b64a
|
|
|
|
|
|
declarations
0e279fe4899beae8630264ef1fe420dd71f29d5d walletdb: Remove unused static functions from walletdb.h (Andrew Chow)
9f536d4fe949666f14a0bf5b814522cecde71f56 wallettool: Have RecoverDatabaseFile return errors and warnings (Andrew Chow)
06e263a4e368671ebb4e4a77c1447ebd5104a488 Call RecoverDatabaseFile directly from wallettool (Andrew Chow)
Pull request description:
Followup to #19324 addressing some comments.
Removes the `SalvageWallet` function in wallettool and instead directly calls `RecoverDatabaseFile` as suggested in https://github.com/bitcoin/bitcoin/pull/19324#discussion_r450379596
Removes the `LogPrintf`s and `tfm::format`s in `RecoverDatabaseFile` as noted in https://github.com/bitcoin/bitcoin/pull/19324#discussion_r448027237
Removes the declarations of `VerifyEnvironment` and `VerifyDatabaseFile` that were forgotten in `walletdb.h` as noted in https://github.com/bitcoin/bitcoin/pull/19324#issuecomment-654389079
ACKs for top commit:
meshcollider:
Code review ACK 0e279fe4899beae8630264ef1fe420dd71f29d5d
ryanofsky:
Code review ACK 0e279fe4899beae8630264ef1fe420dd71f29d5d, just dropped last commit
Tree-SHA512: ffd01f30536c2eab4bf40ba363c3ea916ecef3c8f0c5262040b40498776ffb00f95240204a40e38415d6931800851d0a3fa63ee91efc1d329b60ac317da0363d
|
|
35cd2da623e32b975fbc485c3605934e4aa8bdc5 test: Fix 'make cov' with clang (Hennadii Stepanov)
Pull request description:
This is a follow up of #19688.
With this PR it is possible to do the following:
```
$ ./autogen.sh
$ ./configure --enable-lcov CC=clang CXX=clang++
$ make
$ make cov
```
Currently, on master (8a85377cd0b60cb00dae4f595d628d1afbd28bd5), `make cov` fails to `Processing src/test/test_bitcoin-util_tests.gcda`.
ACKs for top commit:
vasild:
ACK 35cd2da
Crypt-iQ:
ACK 35cd2da
Tree-SHA512: aaf56118e2644064e9738a8279889c617db5805c5c804c904469b24c496bd609f9c5fc2aebcf1a422f8a5ed2eb38bd6e76b484680310b55c36d922b73a4c33cf
|
|
|
|
signing code (sipa)
dca28634d779c775678cba402a85fe5bb9b3a5a9 test: ensure OP_1NEGATE satisfies BIP62 minimal push rule (Jon Atack)
e629d07199b83f4ad313b23a94c9016e3276c52a Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code (Pieter Wuille)
Pull request description:
A rebase of #13084 which additionally modifies the test code (unaddressed in the original, assuming sipa is too busy to deal with this at the moment).
Relatively simple bugfix so it'd be good to have merged soon.
Turning OP_1NEGATE into 0x0181 results in a larger-than-necessary data push instead of just actually using the OP_1NEGATE opcode (0x4f). This fails the minimal push rule of BIP 62 and makes the result non-standard.
ACKs for top commit:
fjahr:
Code review ACK dca28634d779c775678cba402a85fe5bb9b3a5a9
luke-jr:
ACK dca28634d77
jonatack:
ACK dca28634d779c775678cba402a85fe5bb9b3a5a9
Tree-SHA512: 706d9a2ef20c809dea923e477a873e2fd60db8d0ae64289e510b766a38005c1f31ab0b5883f16b9c7863ff0d3f705e8e413f6121320028ac196b79c3184a4113
|
|
f0aa8aeea5a183ea44a877255d12db7732f2e0a8 test: add rpc_generate functional test (Jon Atack)
92d94ffb8d07cc0d2665c901de5903a3a90d5fd0 rpc: print useful help and error message for generate (Jon Atack)
8d32d2011d3f4e1d9e587d6f80dfa4a3e9f9393d test: consider generate covered in _get_uncovered_rpc_commands() (Jon Atack)
Pull request description:
This was a requested follow-up to #19133 and #17700 to alleviate confusion and head-scratching by people following tutorials that use `generate`. See https://github.com/bitcoin/bitcoin/pull/19455#issuecomment-668172916 below, https://github.com/bitcoin/bitcoin/pull/19133#issuecomment-636860943 and https://github.com/bitcoin/bitcoin/pull/17700#issuecomment-566159096.
before
```
$ bitcoin-cli help generate
help: unknown command: generate
$ bitcoin-cli generate
error code: -32601
error message:
Method not found
```
after
```
$ bitcoin-cli help generate
generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information.
$ bitcoin-cli generate
error code: -32601
error message:
generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information.
```
In the general help it remains hidden, as requested by laanwj.
```
$ bitcoin-cli help
== Generating ==
generateblock "output" ["rawtx/txid",...]
generatetoaddress nblocks "address" ( maxtries )
generatetodescriptor num_blocks "descriptor" ( maxtries )
```
ACKs for top commit:
adamjonas:
utACK f0aa8aeea5a183ea44a877255d12db7732f2e0a8
pinheadmz:
ACK f0aa8aeea5a183ea44a877255d12db7732f2e0a8
Tree-SHA512: d083652589ad3e8228c733455245001db22397559c3946e7e573cf9bd01c46e9e88b72d934728ec7f4361436ae4c74adb8f579670b09f479011924357e729af5
|
|
|
|
e7448d66803f42984018ef8dfa6699027cb9536d wallet: Don't override signing errors (Fabian Jahr)
Pull request description:
While reviewing #17204 I noticed that the errors in `input_errors` from `::SignTransaction` where being overridden by `CWallet::SignTransaction`. For example, a Script related error led to incomplete signature data which led to `CWallet::SignTransaction` reporting that keys were missing, which was a less precise error than the original one.
Additionally, the error `"Input not found or already spent"` is [duplicated in `sign.cpp`](https://github.com/bitcoin/bitcoin/blob/c7b4968552c704f1e2e9a046911e1207f5af5fe0/src/script/sign.cpp#L481), so the error here is redundant at the moment. So technically the whole error block could be removed, I think. However, this code is affected by the ongoing work on the wallet so there might be a reason why these errors are here. But even if there is a reason to keep them, I don't think existing, potentially more precise errors should be overridden here unless we want to hide them from the users. I am looking for feedback if this is a work in progress state where these errors could be more useful in the future or if they can be removed.
On testing: even though [the errors in `CWallet` are covered](https://marcofalke.github.io/btc_cov/total.coverage/src/wallet/wallet.cpp.gcov.html), all tests still pass after removing them. I am not sure if there is a desire to cover these specific error messages, tests in `test/functional/rpc_signrawtransaction.py` seem to aim for a more generic approach.
ACKs for top commit:
achow101:
ACK e7448d66803f42984018ef8dfa6699027cb9536d
meshcollider:
Code review ACK e7448d66803f42984018ef8dfa6699027cb9536d
Tree-SHA512: 3e2bc11d05379d2aef87b093a383d1b044787efc70e35955b2f8ecd028b6acef02f386180566af6a1a63193635f5d685466e2f6141c96326c49ffc5c81ca3e23
|
|
fa77de2baa40ee828c850ef4068c76cc3619e87b rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) (MarcoFalke)
fa50bdc755489b2e291ea5ba0e39e44a20c6c6de rpc: Limit echo to 10 args (MarcoFalke)
fa89ca9b5bd334813fd7e7edb202c56b35076e8d refactor: Use C++11 range based for loops to simplify rpc code (MarcoFalke)
fa459bdc87bbb050ca1c8d469023a96ed798540e rpc: Treat all args after a hidden arg as hidden as well (MarcoFalke)
Pull request description:
This is split out from #18531 to just touch the RPC methods in misc. Description from the main pr:
### Motivation
RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.
### Changes
The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
laanwj:
Code review ACK fa77de2baa40ee828c850ef4068c76cc3619e87b
fjahr:
tested ACK fa77de2baa40ee828c850ef4068c76cc3619e87b
theStack:
ACK https://github.com/bitcoin/bitcoin/pull/19528/commits/fa77de2baa40ee828c850ef4068c76cc3619e87b
ryanofsky:
Code review ACK fa77de2baa40ee828c850ef4068c76cc3619e87b. Pretty straightfoward changes
Tree-SHA512: badae1606518c0b55ce2c0bb9025d14f05556532375eb20fd6f3bfadae1e5e6568860bff8599d037e655bf1d23f1f464ca17f4db10a6ab3d502b6e9e61c7b3d3
|
|
f110b7c722eb150816a26cab161ac2b8c0f58609 rpc: document returned error fields as optional if applicable (Sebastian Falbesoner)
Pull request description:
The following RPCs return error fields (named `"error"` or `"errors"`) that are optional, but don't show up as optional in the help text yet:
* `analyzepsbt`
* `estimatesmartfee`
* `signrawtransactionwithkey`
* `signrawtransactionwithwallet`
The following RPC has the errors field already marked as optional, but doesn't match the usual format in the description (like `"if there are any"` in parantheses):
* `estimaterawfee`
This PR adds the missing optional flags and adapts the description strings. Inspired by a recent PR #19634 by justinmoon.
The instances were found via `git grep "RPCResult.*\"error"`. Note that there is one RPC so far where the return error is not optional (i.e. in case of no error, the field is included in the result, but is just empty), namely `bumpfee`.
ACKs for top commit:
adaminsky:
ACK `f110b7c`
laanwj:
ACK f110b7c722eb150816a26cab161ac2b8c0f58609, new documentation looks consistent with actual behavior
achow101:
ACK f110b7c722eb150816a26cab161ac2b8c0f58609
meshcollider:
utACK f110b7c722eb150816a26cab161ac2b8c0f58609
Tree-SHA512: 30c00f78a575b60e32b4536496af986d53a25f33e6ebbf553adcdcf825ad21a44f90267f3d1ea53326dac83bcfa9983fdb3dad6d3126e20f97f3c08ce286e188
|
|
|
|
|
|
|
|
This maintains a persistent list of wallets stored in settings that will
automatically be loaded on startup. Being able to load a wallet automatically
on startup will be more useful in the GUI when the option to create wallets is
added in #15006, but it's reasonable to expose this feature by RPC as well.
|
|
NODE_COMPACT_FILTERS
f5c003d3ead182335252558c5c6c9b9ca8968065 [test] Add test for NODE_COMPACT_FILTER. (Jim Posen)
132b30d9c84f2a8053714a438f227b583a89a9ea [net] Signal NODE_COMPACT_FILTERS if we're serving compact filters. (Jim Posen)
b3fbc94d4f2937bb682f2766cc9a8d4fde328a3f Apply cfilters review fixups (John Newbery)
Pull request description:
If -peerblockfilters is configured, signal the `NODE_COMPACT_FILTERS` service bit to indicate that we are able to serve compact block filters, headers and checkpoints.
ACKs for top commit:
MarcoFalke:
re-review and Concept ACK f5c003d3ead182335252558c5c6c9b9ca8968065
fjahr:
Code review ACK f5c003d3ead182335252558c5c6c9b9ca8968065
clarkmoody:
Concept ACK f5c003d3ead182335252558c5c6c9b9ca8968065
ariard:
Concept and Code Review ACK f5c003d
jonatack:
ACK f5c003d3e
Tree-SHA512: 34d1c153530a0e55d09046fe548c9dc37344b5d6d50e00af1b4e1de1e7b49de770fca8471346a17c151de9fe164776296bb3dd5af331977f0c3ef1e6fc906f85
|
|
count
c133cdcdc3397a734d57e05494682bf9bf6f4c15 Cap listsinceblock target_confirmations param (Adam Stein)
Pull request description:
This addresses an issue brought up in #19587.
Currently, the `target_confirmations` parameter to `listsinceblock` is not checked for being too large. When `target_confirmations` is greater than one more than the current number of blocks, `listsinceblock` fails with error code -1. In comparison, when `target_confirmations` is less than 1, a -8 "Invalid parameter" error code is thrown.
This PR fixes the issue by returning a -8 "Invalid parameter" error if the `target_confirmations` value corresponds to a block with more confirmations than the genesis block. This happens if `target_confirmations` exceeds one more than the number of blocks.
ACKs for top commit:
laanwj:
Code review ACK c133cdcdc3397a734d57e05494682bf9bf6f4c15
ryanofsky:
Code review ACK c133cdcdc3397a734d57e05494682bf9bf6f4c15. Just suggested changes since last review. Thanks!
Tree-SHA512: 02680f4cb937d2c24d5019abd0ebfa188b8a50679a1e64e9c26bfe5c17eef6aea906832e6e2d492ba8a2ea160041bf185d66795ee691e340f6793db03c21b89a
|
|
386ec192a57b76492125d691ceda1b4aa832312e Reduce cs_main lock accumulation during GUI startup (Jonas Schnelli)
d42cb790687788c78aa2f0c1988238ab52050782 Optionally populate BlockAndHeaderTipInfo during AppInitMain (Jonas Schnelli)
b354a1480abbd71fb7fb82c39c81ea0644bbfce4 Add BlockAndHeaderTipInfo to the node interface/appInit (Jonas Schnelli)
25e1d0bf417237caa5d36b4e757f29e6c8be8aad RPCConsole, take initial chaintip data as parameter (Jonas Schnelli)
Pull request description:
During the GUI startup, there is currently an accumulation of cs_main locks due to setting initial chain state values at multiple locations (in the GUI main thread).
This PR tries to cache the initial chain state (tip height, tip time, best header, etc.) short after loading the blockindex.
The cached values are then used instead of fetching them again (and thus locking `cs_main`) during setting the client model.
This should fix the initial GUI blocking often experienced during or short after the splashscreen.
On mac, best tested together with #19007.
ACKs for top commit:
promag:
Code review ACK 386ec192a57b76492125d691ceda1b4aa832312e.
ryanofsky:
Code review ACK 386ec192a57b76492125d691ceda1b4aa832312e. Just rebased since last review due to conflicts
Tree-SHA512: caccca05360e6dc0c3aade5e7ed24be513607821a8bd6612d0337259304ab772799fb2d707a0d7c7e50fbff4bd394354643fd0aeaa3bb55960ccc28562f4763d
|
|
79d6332e9e4fc01e6418247c31e31b4faa1b3b84 moveonly: Fix indentation in bumpfee RPC (Andrew Chow)
431071c28ae35be8aa012df51233be19067d625c Hide bumpfee's psbt creation behavior behind -deprecatedrpc (Andrew Chow)
4638224f64ba7c8ea7c4fb550ec89c6a6d8c7887 Add psbtbumpfee RPC (Andrew Chow)
Pull request description:
Adds a new RPC `psbtbumpfee` which always creates a psbt. `bumpfee` will then only be able to create and broadcast fee bumping transactions instead of changing its behavior based on `IsWalletSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)`.
Split from #18627
ACKs for top commit:
Sjors:
re-utACK 79d6332
meshcollider:
utACK 79d6332e9e4fc01e6418247c31e31b4faa1b3b84
fjahr:
Code review ACK 79d6332e9e4fc01e6418247c31e31b4faa1b3b84
Tree-SHA512: 1c92c4b4461bb30e78be3ee73165f624398ef33996ce36043b61a8931be667030d0fca12fd0b30097b78c56e4e9092c69582b237cbdac51d56f6be23d8c0f1bb
|
|
Previously, listsinceblock would fail with error code -1 when the
target_confirmations exceeded the number of confirmations of the genesis
block. This commit allows target_confirmations to refer to a lastblock
hash with more confirmations than exist in the chain by setting the
lastblock hash to the genesis hash in this case. This allows for
`listsinceblock "" 6` to not fail if the block count is less than 5
which may happen on regtest.
Includes update to the functional test for listsinceblock to test for
this case.
|
|
a51d0ad2de89b9757d158df95ddeba2bfcb23935 rpc: Improve addnode remove command error message (Fabian Jahr)
Pull request description:
The `addnode` RPC with the `remove` command parameter is used to remove a node from the "added nodes". It did not have test coverage and in case of failure to remove the node it responded with the confusing message "Error: Node has not been added.".
This PR adds test coverage and introduces a new error code as well as changes the error message to something that makes sense.
ACKs for top commit:
laanwj:
Code review ACK a51d0ad2de89b9757d158df95ddeba2bfcb23935
theStack:
Tested ACK https://github.com/bitcoin/bitcoin/commit/a51d0ad2de
Tree-SHA512: 033ef5de0d4d49d58ef4df3759b838c9d19ee9dfb0aff9f814a3a63d124ca231a442c930efa7d343fe1f65727c4b59fc23dd5e26fe6ea69f9e84fda48b5c5cc2
|
|
|
|
|
|
|
|
|
|
|