2599d13c9417dc8c5107535521173687ec5e6c2f rpc: Remove deprecated migration code (Vasil Dimov)
Pull request description:
Don't accept a second argument to `sendrawtransaction` and
`testmempoolaccept` of type `bool`. Actually even the code before this
change would not accept `bool`, but it would print a long explanatory
message when rejecting it: "Second argument must be numeric (maxfeerate)
and no longer supports a boolean. To allow a transaction with high fees,
set maxfeerate to 0."
This was scheduled for removal in 6c0a6f73e.
ACKs for top commit:
MarcoFalke:
ACK 2599d13c9417dc8c5107535521173687ec5e6c2f 📅
Tree-SHA512: e2c74c0bde88e20149d0deab0845851bb3979143530a6bae4f46769d61b607ad2e2347f8969093c2461a80c47661732dc0b3def140f8ce84081719adda3b3811
Don't accept a second argument to `sendrawtransaction` and
`testmempoolaccept` of type `bool`. Actually even the code before this
change would not accept `bool`, but it would print a long explanatory
message when rejecting it: "Second argument must be numeric (maxfeerate)
and no longer supports a boolean. To allow a transaction with high fees,
set maxfeerate to 0."
This was scheduled for removal in 6c0a6f73e.
8a2a652e6fab5eb8224beefcc07d9011b61865a8 Remove redundant type information from rpc docs (David O'Callaghan)
Pull request description:
Simple edit of the RPC calls to remove redundant text ("A json object/array ...") from the beginning of help.
Fixes: #18258
Top commit has no ACKs.
Tree-SHA512: cbbf760e0b7b4eda61c40b420ed77f5d878318e37b0eb13e63567212240b2c4ecc15d84030e98075e21c9ae9016539adfd201e5661ea824166a76d335180c32f
fa5c6622c8ecf1954e7177888ad8c97a77b16fb7 doc: Use proper RPC help syntax in importmulti (MarcoFalke)
fab63111bec73859597e6ce0986f76e5e9959091 doc: Remove duplicate "comment" from listsinceblock RPC help (MarcoFalke)
fa04cd6cfc0330b62058ed169d621e08108dc87e doc: Properly document proxy_randomize_credentials as bool in getnetworkinfo (MarcoFalke)
fa9dec7c395897e8dbbb6de7a16ec5185a609d41 doc: Fix syntax error (trailing square bracket) in finalizepsbt (MarcoFalke)
faff5a60ed328d4c5fdef253e8935a351cb57bd0 doc: Fix syntax error (trailing square bracket) in walletprocesspsbt (MarcoFalke)
fa0545901daad32b09511cc61c4af1400c48088d doc: Add missing "optional" to "long" estimaterawfee RPC help (MarcoFalke)
Pull request description:
This fixes documentation of the following RPCs:
* estimaterawfee (hidden)
* https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/walletprocesspsbt/
* https://bitcoincore.org/en/doc/0.19.0/rpc/rawtransactions/finalizepsbt/
* https://bitcoincore.org/en/doc/0.19.0/rpc/network/getnetworkinfo/
* https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/listsinceblock/
* https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/importmulti/
<!-- Also, it comes with a scripted diff to normalize whitespace and type names. (Previous attempts: #14601 and #14459)
ACKs for top commit:
laanwj:
ACK fa5c6622c8ecf1954e7177888ad8c97a77b16fb7
Tree-SHA512: 5a10956e12f8ce23e93a2ce8bafd6cae759d8a21658f79397e3bfce3e4aabd9658bdbd40acde49323dca958a9befee7166654994208c182dd60f483109621e17
deaa6dd144f5650b385658a0c4f9a014aff8dde2 psbt: check output index is within bounds before accessing (Andrew Chow)
f1ef7f0aa46338f4cd8de79696027a1bf868f359 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow)
Pull request description:
Fixes#17149
Two classes of issues were found by the psbt fuzzer: values out of range and causing overflows, and prevout indexes being out of range. This PR fixes both.
When accessing a specific output using the index given in the tx, check that it is actually a possible output before trying to access the output.
When summing and checking amounts for `decodepsbt` and `analyzepsbt`, make sure that the values are actually valid money values.. Otherwise, stop summing and don't show the fee. For `analyzepsbt`, return that the next role is the Creator since the Creator needs to remake the transaction to be valid.
ACKs for top commit:
practicalswift:
ACK deaa6dd144f5650b385658a0c4f9a014aff8dde2 -- only change since last ACK was the addition of tests
gwillen:
tested ACK deaa6dd, would also like to see this merged!
Tree-SHA512: 06c36720bbb5a7ab1c29f7d15878bf9f0d3e5760c06bff479d412e1bf07bb3e0e9ab6cca820a4bfedaab71bfd7af813807e87cbcdf0af25cc3f66a53a06dbcfd
fa8e650b525e9493bdfa393c0c3e34cb22c78c08 rest: Use mempool from node context instead of global (MarcoFalke)
fa660d65d7cc401ad5bbfdc076a074de19a79329 node: Use mempool from node context instead of global (MarcoFalke)
facbaf092f1ab298943206603cff6e6e3d30d452 rpc: Use mempool from node context instead of global (MarcoFalke)
Pull request description:
Currently they are identical, but in the future we might want to turn
the mempool into a unique_ptr. Replacing the global with the mempool
pointer from the node context simplifies this step.
ACKs for top commit:
jnewbery:
Code review ACK fa8e650b5
ryanofsky:
Code review ACK fa8e650b525e9493bdfa393c0c3e34cb22c78c08, Only the discussed REST server changes since the last review.
Tree-SHA512: 0836f3f39cf90306455962918446e5f8612e88c32072b92afc30929aea1f17430bbda0e2b3668d36c9d6b97d63a93cf4903185194571108642b7bf5a39b89125
In decodepsbt if an invalid amount is seen, don't calculate the fee
but still show the invalid value in the decode.
In analyze psbt, if an invalid amount is seen, set the next step to
be the creator as the creator needs to remake the transaction so that
it is valid.
773d4572a4864ab7b6380858d07d9579ff6dd9a2 Mark PSBTs spending unspendable outputs as invalid in analysis (Andrew Chow)
638e40cb6080800c7b0a7f4028f63326acbe4700 Have a PSBTAnalysis state that indicates invalid PSBT (Andrew Chow)
Pull request description:
When analyzing an unspendable PSBT, report that it is unspendable and exit analysis early.
ACKs for top commit:
Sjors:
ACK 773d457
instagibbs:
After some thought ACK 773d4572a4
Tree-SHA512: 99b0cb2fa1ea37593fc65a20effe881639d69ddeeecf5197bc87bc7f2220cbeb40f1d429d517e4d27f2e9fb563a00cd845d2b4b1ce05246a75a6cb56fb9b0ba5
Currently they are identical, but in the future we might want to turn
the mempool into a unique_ptr. Replacing the global with the mempool
pointer from the node context simplifies this step.
Invalid PSBTs need to be re-created, so the next role is the
Creator (new PSBTRole). Additionally, we need to know what went
wrong so an error field was added to PSBTAnalysis.
A PSBTAnalysis indicating invalid will have empty everything,
next will be set to PSBTRole::CREATOR, and an error message.
SignTransaction will be called multiple times in the future. Pass
it a result UniValue so that it can accumulate the results of multiple
SignTransaction passes.
3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5bb1e37b2e6ab2c52791ac05d9271238 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b944f6ad51b3c895837729d3aa56eea [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b3174d19a6d8691ae07e92b32fdfaef11 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e49246822421a7bcc720491427e1dba8a3 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed9afbe5a96caa5f0f4cbec730d27460 [validation] Add CValidationState subclasses (John Newbery)
Pull request description:
Carries out some remaining tidy-ups remaining after PR 15141:
- split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
- various minor code style tidy-ups to the ValidationState class
- remove the useless `ret` parameter from `ValidationState::Invalid()`
- remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
- remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.
Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:
Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.
```sh
git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^
```
After that it's possible to easily see the mechanical changes with:
```sh
git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
```
ACKs for top commit:
laanwj:
ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf
amitiuttarwar:
code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
362ded410b8cb1104b7ef31ff8488fec4824a7d5 Avoid using g_rpc_node global in wallet code (Russell Yanofsky)
8922d7f6b751a3e6b3b9f6fb7961c442877fb65a scripted-diff: Remove g_connman, g_banman globals (Russell Yanofsky)
e6f4f895d5e42feaf7bfa5f41e80292aaa73cd7d Pass NodeContext, ConnMan, BanMan references more places (Russell Yanofsky)
4d5448c76b71c9d91399c31b043237091be2e5e7 MOVEONLY: Move NodeContext struct to node/context.h (Russell Yanofsky)
301bd41a2e6765b185bd55f4c541f9e27aeea29d scripted-diff: Rename InitInterfaces to NodeContext (Russell Yanofsky)
Pull request description:
This change is mainly a naming / organization change intended to simplify #10102. It:
- Renames struct InitInterfaces to struct NodeContext and moves it from
src/init.h to src/node/context.h. This is a cosmetic change intended to make
the point of the struct more obvious.
- Gets rid of BanMan and ConnMan globals making them NodeContext members
instead. Getting rid of these globals has been talked about in past as a way
to implement testing and simulations. Making them NodeContext members is a
way of keeping them accessible without the globals.
- Splits g_rpc_interfaces global into g_rpc_node and g_rpc_chain globals. This
better separates node and wallet rpc methods. Node RPC methods should have
access NodeContext, while wallet RPC methods should only have indirect access
to node functionality via interfaces::Chain.
- Adds NodeContext& references to interfaces::Chain class and the
interfaces::MakeChain() function. This is needed to access ConnMan and BanMan
instances without the globals.
- Gets rid of redundant Node and Chain instances in Qt tests. This is
needed due to the previous MakeChain change, and also makes test setup a
little more straightforward. More cleanup could be done in the future, but it
will require deduplication of bitcoind, bitcoin-qt, and TestingSetup init
code.
ACKs for top commit:
laanwj:
ACK 362ded410b8cb1104b7ef31ff8488fec4824a7d5
Tree-SHA512: 9ae6ff1e33423291d1e52056bac95e0874538390892a6e83c4c115b3c73155a8827c0191b46eb3d14e3b3f6c23ccb08095490880fbc3188026319c71739f7db2
Wallet code should use interfaces::Chain and not directly access to node state.
Add a g_rpc_chain replacement global for wallet code to use, and move
g_rpc_node definition to a libbitcoin_server source file so there are link
errors if wallet code tries to access it.
9075d13153ce06cd59a45644831ecc43126e1e82 [docs] Add release notes for removal of REJECT reasons (John Newbery)
04a2f326ec0f06fb4fce1c4f93500752f05dede8 [validation] Fix REJECT message comments (John Newbery)
e9d5a59e34ff2d538d8f5315efd9908bf24d0fdc [validation] Remove REJECT code from CValidationState (John Newbery)
0053e16714323c1694c834fdca74f064a1a33529 [logging] Don't log REJECT code when transaction is rejected (John Newbery)
a1a07cfe99fc8cee30ba5976dc36b47b1f6532ab [validation] Fix peer punishment for bad blocks (John Newbery)
Pull request description:
We no longer send BIP 61 REJECT messages, so there's no need to set
a REJECT code in the CValidationState object.
Note that there is a minor bug fix in p2p behaviour here. Because the
call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only
previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`,
then there are cases were `MaybePunishNode()` can get called where it
wasn't previously:
- when `AcceptBlockHeader()` fails with `CACHED_INVALID`.
- when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`.
Note that `BlockChecked()` cannot fail with an 'internal' reject code. The
only internal reject code was `REJECT_HIGHFEE`, which was only set in
ATMP.
This reverts a minor bug introduced in 5d08c9c579ba8cc7b684105c6a08263992b08d52.
ACKs for top commit:
ariard:
ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits
fjahr:
ACK 9075d13153ce06cd59a45644831ecc43126e1e82, confirmed diff to last review was fixing nits in docs/comments.
ryanofsky:
Code review ACK 9075d13153ce06cd59a45644831ecc43126e1e82. Only changes since last review are splitting the main commit and updating comments
Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7
85016e52f6adc01735beefe5a8aefcc4f0c4aa25 [rpc] Fix broken bitcoin-cli examples (Andrew Toth)
Pull request description:
This fixes the `bitcoin-cli` examples for `combinerawtransaction`, `combinepsbt` and `testmempoolaccept`. They currently return `Error parsing JSON`.
ACKs for top commit:
laanwj:
ACK 85016e52f6adc01735beefe5a8aefcc4f0c4aa25
Tree-SHA512: b561f68f7a188dc91dab1ceb98da3ac3e232143ab2b906c90f95c6b74b584599d0f3b51f067cdd3b1153931f95b3dc385e453b1a0dde86f9cb549b94560f219d
Remove the BIP61 REJECT code from error messages and logs when a
transaction is rejected.
BIP61 support was removed from Bitcoin Core in
fa25f43ac5692082dba3f90456c501eb08f1b75c. The REJECT codes will be
removed from the codebase entirely in the following commit.
2dfd6834ef8737e16e4b96df0c459f30a0721d6c test: Add test for default maxfeerate in sendrawtransaction (Joonmo Yang)
261843e4bef96ab296a9775819a99bfa60cad743 wallet/rpc: Use the default maxfeerate value as BTC/kB (Joonmo Yang)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/16382
This patch tries to treat `maxfeerate` in sendrawtransaction/testmempoolaccept RPC as a rate(BTC/kB) instead of an absolute value(BTC).
The included test case checks if the new behavior works correctly, by using the transaction with an absolute fee of ~0.02BTC, where the fee rate is ~0.2BTC/kB.
This test should be failing if the default `maxfeerate` is 0.1BTC, but pass if the default value is 0.1BTC/kB
ACKs for top commit:
laanwj:
ACK 2dfd6834ef8737e16e4b96df0c459f30a0721d6c (ACKs by Sjors and MarcoFalke above for trivially different code)
Tree-SHA512: a1795bffe8a182acef8844797955db1f60bb0c0ded97148f3572dc265234d5219271a3a7aa0b6418a43f73b2b2720ef7412ba169c99bb1cdcac52051f537d6af
c0b5d9710322a614a50ab5da081558cf6a38ad2a Test that joinpsbts randomly shuffles the inputs (Andrew Chow)
6f405a1d3b38395e35571b68aae55cae50e0762a Shuffle inputs and outputs after joining psbts (Andrew Chow)
Pull request description:
`joinpsbts` currently just adds the inputs and outputs in the order of that the PSBTs were provided. This makes it extremely easy to identify which outputs belong to which inputs. This PR changes that so that all of the inputs and outputs are shuffled in the joined transaction.
ACKs for top commit:
instagibbs:
utACK c0b5d97103
jonatack:
ACK c0b5d9710322a614a50ab5da081558cf6a38ad2a modulo suggestions for later.
Tree-SHA512: 14a0b7aae07d92e6d2c76a3a3b228b481e1964cb7d34f97515bdda18e2ea05a9f97c5a22affc143b86ae8b95c3cb239849fb54219d65512bc2112264dca915c8
fb62f128bbfd8c6cd72ea8e23331a4bae23883ab Tidy up BroadcastTransaction() (John Newbery)
b8eecf8e79dad92ff07b851b1b29c2a66546bbc1 Remove unused submitToMemoryPool and relayTransactions Chain interfaces (Antoine Riard)
8753f5652b4710e66b50ce87788bf6f33619b75a Remove duplicate checks in SubmitMemoryPoolAndRelay (Antoine Riard)
611291c198eb2be9bf1aea1bf9b2187b18bdb3aa Introduce CWalletTx::SubmitMemoryPoolAndRelay (Antoine Riard)
8c8aa19b4b4fa56cd359092ef099bcfc7b26c334 Add BroadcastTransaction utility usage in Chain interface (Antoine Riard)
Pull request description:
Remove CWalletTx::AcceptToMemoryPool
Replace CWalletTx::RelayWalletTransaction by SubmitMemoryPoolAndRelay
Add a relay flag to broadcastTransaction because wasn't sure of ReacceptWalletTransactions semantic.
Obviously, working on implementing https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984 to add the new higher-method in Node interface, will add a commit, just need more thought to do it cleanly
ACKs for top commit:
MarcoFalke:
re-ACK fb62f128bbfd8c6cd72ea8e23331a4bae23883ab
Sjors:
re-ACK fb62f128bbfd8c6cd72ea8e23331a4bae23883ab
Tree-SHA512: a7ee48b0545f537fa65cac8ed4cb24e777ab90b877d4eefb87971fa93c6a59bd555b62ad8940c6ffb40592a0bd50787d27587af99f20b56af72b415b6394251f
d6b3640ac732f6f66a8cb6761084d1beecc8a876 [test] walletcreatefundedpsbt: check RBF is disabled when -walletrbf=0 (Sjors Provoost)
9ed062b5685eb6227d694572cb0f7bfbcc151b36 [doc] rpc: remove "fallback to" from RBF default help (Sjors Provoost)
4fcb698bc2bb74171cd3a14b94f9882d8e19e9fb [rpc] walletcreatefundedpsbt: use wallet default RBF (Sjors Provoost)
Pull request description:
The `walletcreatefundedpsbt` RPC call currently ignores `-walletrbf` and defaults to not use RBF. This PR fixes that.
This PR also replaces UniValue in `ConstructTransaction` with a `bool` in preparation of moving this helper method out of the RPC codebase entirely. This may be a bit overkill, but does slightly simplify it.
Fixes#15878
ACKs for top commit:
achow101:
Code Review ACK d6b3640ac732f6f66a8cb6761084d1beecc8a876
l2a5b1:
re-ACK d6b3640
MarcoFalke:
ACK d6b3640ac732f6f66a8cb6761084d1beecc8a876
Tree-SHA512: 55b9bccd1ef36b54f6b34793017dc0721103099ad3761b3b04862291ee13d6915915d4dbb1a8567924fa56e5e95dfe10eec070e06701610e70c87f8ea92b2a00
Access through a broadcastTransaction method.
Add a wait_callback flag to turn off race protection when wallet
already track its tx being in mempool
Standardise highfee, absurdfee variable name to max_tx_fee
We drop the P2P check in BroadcastTransaction as g_connman is only
called by RPCs and the wallet scheduler, both of which are initialized
after g_connman is assigned and stopped before g_connman is reset.
93ce4a0b6fb54efb1f424a71dfc09cc33307e5b9 Move WatchOnly stuff from SigningProvider to CWallet (Andrew Chow)
8f5b81e6edae9cb22559545de63f391d97c15701 Remove CCryptoKeyStore and move all of it's functionality into CWallet (Andrew Chow)
37a79a4fccbf6cd65a933594e24e59d36e674653 Move various SigningProviders to signingprovider.{cpp,h} (Andrew Chow)
16f8096e911e4d59292240a17e2d4004f0500b9e Move KeyOriginInfo to its own header file (Andrew Chow)
d9becff4e13da8e182631baa79b9794c03d44434 scripted-diff: rename CBasicKeyStore to FillableSigningProvider (Andrew Chow)
a913e3f2fbeb1352fc66f334d4f5f7332ea89ad7 Move HaveKey static function from keystore to rpcwallet where it is used (Andrew Chow)
c7797ec65544bd23a2e571b2892e1bf512f2a485 Remove CKeyStore and squash into CBasicKeyStore (Andrew Chow)
1b699a5083b435c2b79f3951f94ac9f967d24f6c Add HaveKey and HaveCScript to SigningProvider (Andrew Chow)
Pull request description:
This PR compresses the `CWallet` chain of inheritance from 5 classes to 3 classes. `CBasicKeyStore` is renamed to `FillableSigningProvider` and some parts of it (the watchonly parts) are moved into `CWallet`. `CKeyStore` and `CCrypoKeyStore` are completely removed. `CKeyStore`'s `Have*` functions are moved into `SigningProvider` and the `Add*` moved into `FillableSigningProvider`, thus allowing it to go away entirely. `CCryptoKeyStore`'s functionality is moved into `CWallet`. The new inheritance chain is:
```
SigningProvider -> FillableSigningProvider -> CWallet
```
`SigningProvider` now is the class the provides keys and scripts and indicates whether keys and scripts are present. `FillableSigningProvider` allows keys and scripts to be added to the signing provider via `Add*` functions. `CWallet` handles all of the watchonly stuff (`AddWatchOnly`, `HaveWatchOnly`, `RemoveWatchOnly` which were previously in `CKeyStore`) and key encryption (previously in `CCryptoKeyStore`).
Implements the 2nd [prerequisite](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#cwallet-subclass-stack) from the wallet restructure.
ACKs for top commit:
Sjors:
re-ACK 93ce4a0; it keeps `EncryptSecret`, `DecryptSecret` and `DecryptKey` in `wallet/crypter.cpp`, but makes them not static. It improves alphabetical includes, reorders some function definitions, fixes commit message, brings back lost code comment.
instagibbs:
utACK 93ce4a0b6f
Tree-SHA512: 393dfd0623ad2dac38395eb89b862424318d6072f0b7083c92a0d207fd032c48b284f5f2cb13bc492f34557de350c5fee925da02e47daf011c5c6930a721b6d3
