3025ca9e77 [net processing] Add RemovePeer() (John Newbery)
a20ab22786 [net processing] Make GetPeerRef const (John Newbery)
ed7e469cee [net_processing] Move peer_map to PeerManager (John Newbery)
a529fd3e3f [net processing] Move GetNodeStateStats into PeerManager (John Newbery)
Pull request description:
This moves `g_peer_map` from a global in net_processing.cpp's unnamed namespace to being a member `m_peer_map` of `PeerManager`.
ACKs for top commit:
theuni:
Re-ACK 3025ca9e77.
dongcarl:
Re-ACK 3025ca9
hebasto:
re-ACK 3025ca9e77, since my [previous](https://github.com/bitcoin/bitcoin/pull/19910#pullrequestreview-545574237) review only reverted the change that introduced NRVO in `PeerManager::GetPeerRef`, and comments are fixed in the proper commits.
Tree-SHA512: 6369eb3c688ac5b84f89f7674115f78ff02edbed76063ac2ebb1759894c9e973883e10821a35dab92bd3d738280acc095bd5368f552a060b83cd309330387d47
fa8abdc995 rpc: Use FeeModes doc helper in estimatesmartfee (MarcoFalke)
Pull request description:
Not sure why this doesn't use the doc helper, probably an oversight?
ACKs for top commit:
laanwj:
Code review ACK fa8abdc995
Tree-SHA512: 1f2dc8356e3476ddcf9cafafa7f9865ad95bed1e3067c0edab8e3c483e374bdbdbecc066167554b4a1b479e28f6a52c4ae6a75a70c67ee4e1ff4f3ba36b04001
4e28753f60 feestimator: encapsulate estimation file logic (Antoine Poinsot)
e8ea6ad9c1 init: don't create a CBlockPolicyEstimator if we don't relay transactions (Antoine Poinsot)
86ff2cf202 Remove the remaining fee estimation globals (Antoine Poinsot)
03bfeee957 interface: remove unused estimateSmartFee method from node (Antoine Poinsot)
Pull request description:
If the `blocksonly` mode is turned on after running with transaction
relay enabled for a while, the fee estimation will serve outdated data
to both the internal wallet and to external applications that might be
feerate-sensitive and make use of `estimatesmartfee` (for example a
Lightning Network node).
This has already caused issues (for example https://github.com/bitcoin/bitcoin/issues/16840 (C-lightning), or https://github.com/lightningnetwork/lnd/issues/2562 (LND)) and it seems prudent to fail rather than to give inaccurate values.
This fixes#16840, and closes#16890 which tried to fix the symptoms (RPC) but not the cause as mentioned by sdaftuar :
> If this is a substantial problem, then I would think we should take action to protect our own wallet users as well (rather than hide the results of what our fee estimation would do!).
ACKs for top commit:
MarcoFalke:
re-ACK 4e28753f60👋
jnewbery:
utACK 4e28753f60
Tree-SHA512: c869cf03b86d8194002970bbc84662dae76874967949b9be0d9a4511a1eabcb1627c38aca3154da9dcece1a4c49ec02bd4f9fcca2ec310986e07904559e63ba8
This moves the CBlockPolicyEstimator to the NodeContext, which get rids
of two globals and allows us to conditionally create the
CBlockPolicyEstimator (and to remove a circular dep).
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
faaf9c58e4 remove CRPCCommand constructor that takes rpcfn_type function pointer (MarcoFalke)
fa19bb2cd8 remove dead rpc code (MarcoFalke)
Pull request description:
Remove the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
### 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 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:
fjahr:
tested ACK faaf9c58e4
promag:
Tested ACK faaf9c58e4.
ryanofsky:
Code review ACK faaf9c58e4. Two obviously good simplifications.
Tree-SHA512: 5de3b440f7b2ed2c3e86655d4f0e2e5df9c67e8ce3c7817d5ea5311d1a38690f2f3e28fab41aad6936be9fc884326d037e5f19e85d4d2fe281474dada13911ee
Create a fee_rate (sat/vB) RPC param and replace overloading the conf_target and
estimate_mode params in the following 6 RPCs with it:
- sendtoaddress
- sendmany
- send
- fundrawtransaction
- walletcreatefundedpsbt
- bumpfee
In RPC bumpfee, the previously existing fee_rate remains but the unit is changed
from BTC/kvB to sat/vB. This is a breaking change, but it should not be an
overly risky one, as the units change by a factor of 1e5 and any fees specified
in BTC/kvB after this commit will either be too low and raise an error or be 1
sat/vB and can be RBFed.
Update the test coverage for each RPC.
Co-authored-by: Murch <murch@murch.one>
0be29000c0 rpc: update conf_target helps for correctness/consistency (Jon Atack)
778b9be406 wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack)
603c005083 wallet: add rpc send explicit fee rate coverage (Jon Atack)
dd341e602d wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack)
44e7bfa603 wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack)
6e1ea4273e test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack)
3ac7b0c6f1 wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack)
2d8eba8f84 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack)
1697a40b6f wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack)
fc5721723d wallet: fix SetFeeEstimateMode() error message (Jon Atack)
052427eef1 wallet, bugfix: fix bumpfee with explicit fee rate modes (Jon Atack)
Pull request description:
Follow-up to #11413 providing a base to build on for #19543:
- bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue #20219
- adds explicit feerate test coverage for `bumpfee`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendtoaddress`, and `sendmany`
- improves a few related RPC error messages and `ParseConfirmTarget()` / error message
- fixes/improves the explicit fee rate information in the 6 RPC helps, of which 2 were also missing `conf_target` sat/B units
This provides a spec and regression coverage for the potential next step of a universal `sat/vB` feerate argument (see #19543), as well as immediate coverage and minimum fixes for 0.21.
ACKs for top commit:
kallewoof:
Concept/Tested ACK 0be29000c0
meshcollider:
Code review + functional test run ACK 0be29000c0
Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0
3333077823 rpc: Adjust witness-tx deserialize error message (MarcoFalke)
cccc752569 rpc: Properly deserialize txs with witness before signing (MarcoFalke)
Pull request description:
Signing a transaction can only happen when the transaction has inputs. A transaction with inputs can always be deserialized as witness-transaction. If `try_no_witness` decoding is attempted, this will lead to rare intermittent failures.
Fixes#18803
ACKs for top commit:
achow101:
ACK 3333077823
ajtowns:
ACK 3333077823
Tree-SHA512: 73f8a5cdfe03fb0e68908d2fa09752c346406f455694a020ec0dd1267ef8f0a583b8e84063ea74aac127106dd193b72623ca6d81469a94b3f5b3c766ebf2c42b
41dca087b7 [trivial] Extract connection type doc into file where it is used. (Amiti Uttarwar)
3069b56a45 [doc] Improve help for getpeerinfo connection_type field. (Amiti Uttarwar)
Pull request description:
two commits addressing small followups from #19725
* first commit adds a clarification in the release notes that this field shouldn't be expected to be stable (suggested by sdaftuar in https://github.com/bitcoin/bitcoin/pull/19725#issuecomment-697421878)
* second commit moves the `CONNECTION_TYPE_DOC` object out of the header file to reduce the size of the binary (suggested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/19725#discussion_r495467895, he tested and found a decrease of 10kB)
ACKs for top commit:
achow101:
ACK 41dca087b7
laanwj:
Code review ACK 41dca087b7
Tree-SHA512: a555df978b4341fbe05deeb40a8a655f0d3c5c1c0adcc1737fd2cf61b204a5a24a301ca0c2b5a3616554d4abf8c57074d22dbda5a50d8450bc22c57679424985
6272604bef refactor: enable -netinfo to add future networks (i2p, cjdns) (Jon Atack)
82fd40216c refactor: promote some -netinfo localvars to class members (Jon Atack)
5133fab37e cli: simplify -netinfo using getpeerinfo network field (Jon Atack)
4938a109ad rpc, test: expose CNodeStats network in RPC getpeerinfo (Jon Atack)
6df7882029 net: add peer network to CNodeStats (Jon Atack)
Pull request description:
This PR:
- builds on #19991 and #19998
- exposes peer networks via a new getpeerinfo `network` field ("ipv4", "ipv6", or "onion"), and adds functional tests
- updates -netinfo to use getpeerinfo `network` rather than detecting the peer networks client-side
- refactors -netinfo to easily add future networks
ACKs for top commit:
laanwj:
ACK 6272604bef
Tree-SHA512: 28883487585135ceaaf84ce09131f2336e3193407f2e3df0960e3f4ac340f500ab94ffecb9d06a4c49bc05e3cca4f914ea4379860bea0bd5df2f834f74616015
5b57dc5458 RPC: getpeerinfo: Wrap long help line for bytesrecv_per_msg (Luke Dashjr)
d681a28219 RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") (Luke Dashjr)
Pull request description:
If we were going to continue support for "whitelisted", we should have probably made it true if any permission flag was set, rather than only if "default permissions" were used.
This corrects the description, and deprecates it.
ACKs for top commit:
laanwj:
ACK 5b57dc5458
Tree-SHA512: a2e2137f8be8110357c1b2fef2c923fa8c7c4a49b0b2b3a2d78aedf12f8ed5cc7e140018a21b37e6ec7770ed4007542aeef7ad4558973901b107e8e0f81d6003
0e2a5e448f tests: dumping and minimizing of script assets data (Pieter Wuille)
4567ba034c tests: add generic qa-asset-based script verification unit test (Pieter Wuille)
f06e6d0345 tests: functional tests for Schnorr/Taproot/Tapscript (Pieter Wuille)
3c226639eb tests: add BIP340 Schnorr signature support to test framework (Pieter Wuille)
206fb180ec --- [TAPROOT] Tests --- (Pieter Wuille)
d7ff237f29 Activate Taproot/Tapscript on regtest (BIP 341, BIP 342) (Pieter Wuille)
e9a021d7e6 Make Taproot spends standard + policy limits (Pieter Wuille)
865d2c37e2 --- [TAPROOT] Regtest activation and policy --- (Pieter Wuille)
72422ce396 Implement Tapscript script validation rules (BIP 342) (Johnson Lau)
330de894a9 Use ScriptExecutionData to pass through annex hash (Pieter Wuille)
8bbed4b7ac Implement Taproot validation (BIP 341) (Pieter Wuille)
0664f5fe1f Support for Schnorr signatures and integration in SignatureCheckers (BIP 340) (Pieter Wuille)
5de246ca81 Implement Taproot signature hashing (BIP 341) (Johnson Lau)
9eb590894f Add TaggedHash function (BIP 340) (Pieter Wuille)
450d2b2371 --- [TAPROOT] BIP340/341/342 consensus rules --- (Pieter Wuille)
5d62e3a68b refactor: keep spent outputs in PrecomputedTransactionData (Pieter Wuille)
8bd2b4e784 refactor: rename scriptPubKey in VerifyWitnessProgram to exec_script (Pieter Wuille)
107b57df9f scripted-diff: put ECDSA in name of signature functions (Pieter Wuille)
f8c099e220 --- [TAPROOT] Refactors --- (Pieter Wuille)
Pull request description:
This is an implementation of the Schnorr/taproot consensus rules proposed by BIPs [340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), [341](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), and [342](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki).
See the list of commits [below](https://github.com/bitcoin/bitcoin/pull/19953#issuecomment-691815830). No signing or wallet support of any kind is included, as testing is done entirely through the Python test framework.
This is a successor to https://github.com/bitcoin/bitcoin/pull/17977 (see discussion following [this comment](https://github.com/bitcoin/bitcoin/pull/17977#issuecomment-682285983)), and will have further changes squashed/rebased. The history of this PR can be found in #19997.
ACKs for top commit:
instagibbs:
reACK 0e2a5e448f
benthecarman:
reACK 0e2a5e4
kallewoof:
reACK 0e2a5e448f
jonasnick:
ACK 0e2a5e448f almost only looked at bip340/libsecp related code
jonatack:
ACK 0e2a5e448f modulo the last four commits (tests) that I plan to finish reviewing tomorrow
fjahr:
reACK 0e2a5e448f
achow101:
ACK 0e2a5e448f
Tree-SHA512: 1b00314450a2938a22bccbb4e177230cf08bd365d72055f9d526891f334b364c997e260c10bc19ca78440b6767712c9feea7faad9a1045dd51a5b96f7ca8146e
b048b275d9 [validation] Remove absurdfee from accepttomempool (John Newbery)
932564b9cf scripted-diff: update max-fee-exceeded error message to include RPC (gzhao408)
8f1290c601 [rpc/node] check for high fee before ATMP in clients (gzhao408)
Pull request description:
Picked up from #15810. Add separate fee-checking logic for clients that need to enforce max fee rates, then remove the `absurdFee` logic from ATMP.
ATMP's `nAbsurdFee` argument is used to enforce user-specific behavior (it is not policy since it isn't applied consistently: it is only ever used in RPC and wallet, and set to 0 everywhere else internally). It should be removed from `AcceptToMemoryPool` because (1) validation results/mempool behavior should not be user-specific and (2) enforcing a max fee rate should be the responsibility of the client instead of the mempool.
Note: this PR does not intend to _remove_ protection from high fees, just re-delegate the responsibility to clients.
ACKs for top commit:
jnewbery:
utACK b048b275d9
LarryRuane:
re-ACK b048b275d9
MarcoFalke:
re-ACK b048b275d9 , only change is squashing one commit 🏦
instagibbs:
utACK b048b275d9
Tree-SHA512: 57c17ba16d230a4cae2896dd6a64c924f307757824e35784bf96da7b10aff2d8ea910710edf35e981035623a155f8766209a92a0fdb856549fde78bc3eaae4d2
Check absurd fee in BroadcastTransaction and RPC,
return TransactionError::MAX_FEE_EXCEEDED instead
of TxValidationResult::TX_NOT_STANDARD because this
is client preference, not a node-wide policy.
1885ad3546 RPC: remove duplicate line in getblock help (Fabian Jahr)
Pull request description:
Line simply seems duplicated in error.
Testing instructions:
Run `src/bitcoin-cli help getblock` on master branch to reproduce. Then build this PR and compare its results.
ACKs for top commit:
dhruv:
tACK `1885ad3`
kristapsk:
ACK 1885ad3546
Emzy:
tACK 1885ad3546
Tree-SHA512: 870c035cb553b0e1d5ef72e64231ef277e0392efe94bc6ecf47129023bd94a6d5a276f46529807f68a1db55c7baa94d9119c7264d9947bc4e5dd9dcefd1b13e7
f471a3be00 scripted diff: Improve invalid vout value rpc error message (Nima Yazdanmehr)
Pull request description:
Since the `vout` value can start at `0`, the error message for *negative* values can be improved to something like: `vout cannot be negative`.
ACKs for top commit:
fanquake:
ACK f471a3be00
promag:
Code review ACK f471a3be00.
Tree-SHA512: fbdee3d0ddd5b58eb93934a1217b44e125a9ad39e672b1f35c7609c6c5fcf45ae1b731d3d6135b7225d98792dbfc34a50907b8c41274a5b029d7b5c59f886560
-BEGIN VERIFY SCRIPT-
r() { sed -i 's/vout must be positive/vout cannot be negative/g' $1 }
r $(git grep -l 'vout must be positive')
-END VERIFY SCRIPT-
69cf5d4eeb [test] Make sure send rpc returns fee reason (Sishir Giri)
d5863c0b3e [send] Make send RPCs return fee reason (Sishir Giri)
Pull request description:
Whenever a wallet funds a transaction, the fee reason is reported to the user only if the verbose is set to true. I added an extra parameter to `CreateTransaction` function in wallet.cpp. Then I implemented the fee reason return logic in `SendMoney` in rpcwallet.cpp, followed by verbose parameter in `sendtoaddress` and `sendmany` functions. I also added a fee reason test case in walletbasic.py.
link to the issue: https://github.com/MarcoFalke/bitcoin-core/issues/22#issue-616251578
ACKs for top commit:
instagibbs:
ACK 69cf5d4eeb
meshcollider:
utACK 69cf5d4eeb
Tree-SHA512: 2e3af32dcfbd5511ba95f8bc8edca7acfe709a8430ff03e43172e5d0af3dfa4b2f57906978e7f272d878043b9ed8c6004674cf47d7496b005d5f612e9a58aa0e
f7b331ea85 rpc: add brackets to ConstructTransaction (Sjors Provoost)
d813d26f06 [rpc] send: various touch-ups (Sjors Provoost)
0fc1c685e1 [rpc] send: fix parsing replaceable option (Sjors Provoost)
efc9b85e6f Mark send RPC experimental (Sjors Provoost)
Pull request description:
Followup based on #16378 nits. It also fixes an argument parsing error (uncaught because the test wasn't sufficiently thorough).
I marked the RPC as experimental so we can tweak it a bit over the next release cycle.
ACKs for top commit:
meshcollider:
utACK f7b331ea85
fjahr:
utACK f7b331ea85
kallewoof:
ACK f7b331ea85
Tree-SHA512: 82dd8ac76a6558872db3f5249d4d6440469400aaa339153bc627d1ee673a91ecfadecb486bc1939ba87ebbd80e26ff29698e93e358599f3d26fde0e526892afe
a512925e19 [doc] Release notes (Amiti Uttarwar)
50f94b34a3 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar)
df091b9b50 [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar)
395acfa83a [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar)
49c10a9ca4 [log] Add connection type to log statement (Amiti Uttarwar)
Pull request description:
After #19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field.
This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string.
Suggested by sdaftuar- https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468001604 & https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468018093
ACKs for top commit:
jnewbery:
Code review ACK a512925e19.
sipa:
utACK a512925e19
guggero:
Tested and code review ACK a512925e.
MarcoFalke:
cr ACK a512925e19🌇
promag:
Code review ACK a512925e19.
Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
Checking for fHelp, or the size of the args, is dead code because:
* fHelp is always false (src/qt/test/rpcnestedtests.cpp)
* It is already implicitly called by RPCHelpMan::Check
(src/rpc/mining.cpp, src/rpc/misc.cpp, src/rpc/net.cpp)
fa14f57fbc Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) (MarcoFalke)
Pull request description:
This is the last part split out from #18531 to just touch some RPC methods. 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:
fjahr:
tACK fa14f57fbc
ryanofsky:
Code review ACK fa14f57fbc. Just straightforward replacements except code moved in `addnode`, and displatching updated in `bumpfee_helper`
Tree-SHA512: e07af150f1d95a88e558256ce197a6b7dc6cd722a6d6c13c75d944c49c2e2441f8b8237e9f94b03db69fa18f9bda627b0781d5e1da70bf5415e09b38728a8cb1
Using the zmq notifications to avoid excessive mempool polling can be difficult
given the current notifications available. It announces all transactions
being added to mempool or included in blocks, but announces no evictions
and gives no indication if the transaction is in the mempool or a block.
Block notifications for zmq are also substandard, in that it only announces
block tips, while all block transactions are still announced.
This commit adds a unified stream which can be used to closely track mempool:
1) getrawmempool to fill out mempool knowledge
2) if txhash is announced, add or remove from set
based on add/remove flag
3) if blockhash is announced, get block txn list,
remove from those transactions local view of mempool
4) if we drop a sequence number, go to (1)
The mempool sequence number starts at the value 1, and
increments each time a transaction enters the mempool,
or is evicted from the mempool for any reason, including
block inclusion. The mempool sequence number is published
via ZMQ for any transaction-related notification.
These features allow for ZMQ/RPC consumer to track mempool
state in a more exacting way, without unnecesarily polling
getrawmempool. See interface_zmq.py::test_mempool_sync for
example usage.
