The migration process reloads the wallet after all failures.
This commit tests the behavior by trying to obtain a new address
after a decryption failure during migration.
The "connect to ourself" detection logic has been first introduced
by Satoshi in October 2009, together with a couple of other changes
and a version bump to "v0.1.6 BETA" (see commit
cc0b4c3b62).
73f0a6cbd0 doc: detail -rpccookieperms option (willcl-ark)
d2afa2690c test: add rpccookieperms test (willcl-ark)
f467aede78 init: add option for rpccookie permissions (willcl-ark)
7df03f1a92 util: add perm string helper functions (willcl-ark)
Pull request description:
This PR picks up #26088 by aureleoules which adds a bitcoind launch option `-rpccookieperms` to set the file permissions of the cookie generated by bitcoin core.
Example usage to make the generated cookie group-readable: `./src/bitcoind -rpccookieperms=group`.
Accepted values for `-rpccookieperms` are `[owner|group|all]`. We let `fs::perms` handle platform-specific permissions changes.
ACKs for top commit:
achow101:
ACK 73f0a6cbd0
ryanofsky:
Code review ACK 73f0a6cbd0. Main change since last review is no longer throwing a skip exception in the rpc test on windows, so other checks can run after it, and overall test result is passing, not skipped. Also were clarifying renames and documentation improvements.
tdb3:
cr ACK 73f0a6cbd0
Tree-SHA512: e800d59a44aca10e1c58ca69bf3fdde9f6ccf5eab4b7b962645af6d6bc0cfa3a357701e409c8c60d8d7744fcd33a91e77ada11790aa88cd7811ef60fab86ab11
This allows a transaction's weight to be bound under a certain
weight if possible and desired. This can be beneficial for future
RBF attempts, or whenever a more restricted spend topology is
desired.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
72b226882f wallet: notify when preset + automatic inputs exceed max weight (furszy)
Pull request description:
Small change. Found it while finishing my review on #29523. This does not interfere with it.
Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight, but we are not doing so when the user-preselected inputs combined with the wallet-selected inputs exceed the maximum weight.
This change avoids signing all inputs before erroring out and introduces test coverage for `fundrawtransaction`.
ACKs for top commit:
achow101:
ACK 72b226882f
tdb3:
re ACK for 72b226882f
rkrux:
tACK [72b2268](72b226882f)
ismaelsadeeq:
utACK 72b226882f
Tree-SHA512: d77be19231023383a9c79a5d66b642dcbc6ebfc31a363e0b9f063c44898720a7859ec211cdbc0914ac7a3bfdf15e52fb8fc20d97f171431f70492c0f159dbc36
a9716c53f0 rpc: call IsInitialBlockDownload via miner interface (Sjors Provoost)
dda0b0834f rpc: minize getTipHash() calls in gbt (Sjors Provoost)
7b4d3249ce rpc: call processNewBlock via miner interface (Sjors Provoost)
9e228351e7 rpc: getTransactionsUpdated via miner interface (Sjors Provoost)
64ebb0f971 Always pass options to BlockAssembler constructor (Sjors Provoost)
4bf2e361da rpc: call CreateNewBlock via miner interface (Sjors Provoost)
404b01c436 rpc: getblocktemplate getTipHash() via Miner interface (Sjors Provoost)
d8a3496b5a rpc: call TestBlockValidity via miner interface (Sjors Provoost)
8ecb681678 Introduce Mining interface (Sjors Provoost)
Pull request description:
Introduce a `Mining` interface for the `getblocktemplate`, `generateblock` and other mining RPCs to use now, and for Stratum v2 to use later.
Suggested here: https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652
The selection of methods added to the interface is mostly based on what the Template Provider in #29432 uses. It could be expanded further so that `rpc/mining.cpp` no longer needs `EnsureMemPool` and `EnsureChainman`.
This PR should be a pure refactor.
ACKs for top commit:
tdb3:
re ACK a9716c53f0
itornaza:
Code review and std-tests ACK a9716c53f0
ryanofsky:
Code review ACK a9716c53f0 with one minor suggestion in case you update. Only changes since last review were other small changes to the interface.
Tree-SHA512: cf97f87d6e9ed89da3835a0730da3b24a7b14c8605ea221149103a5915e79598cf082a95f2bc88e33f1c450e3d4aad88aed1163a29195acca88bcace055af724
This test type is represented using SEND_NO_AAD. If AAD of the first encrypted packet
sent after the garbage terminator (optional decoy packet/version packet) hasn't been
filled, disconnection happens.
This test type is represented using WRONG_GARBAGE.
Here, garbage bytes sent to TestNode are assumed to be tampered with and
do not correspond to the garbage bytes which P2PInterface calculated and
uses.
This test type is represented using WRONG_GARBAGE_TERMINATOR.
since the wrong garbage terminator is sent to TestNode, TestNode
will interpret all of the gabage bytes, wrong garbage terminator,
decoy messages and version packet it receives as garbage bytes.
If the length of all these is more than 4095 + 16, it will result
in a missing garbage terminator error. otherwise, it will result
in a V2 handshake timeout error.
Send only MAX_GARBAGE_LEN//2 bytes of garbage data to TestNode
so that the total length received by the TestNode is at max
= (MAX_GARBAGE_LEN//2) + 16 + 10*120 + 20 = 3283 bytes
(which is less than 4095 + 16 bytes) and we get a consistent
V2 handshake timeout error message.
If we do not limit the garbage length sent, we will intermittently
get both missing garbage terminator error and V2 handshake
timeout error based on the garbage length and decoy packets length
which are chosen at random.
6eecba475e net_processing: make MaybePunishNodeFor{Block,Tx} return void (Pieter Wuille)
ae60d485da net_processing: remove Misbehavior score and increments (Pieter Wuille)
6457c31197 net_processing: make all Misbehaving increments = 100 (Pieter Wuille)
5120ab1478 net_processing: drop 8 headers threshold for incoming BIP130 (Pieter Wuille)
944c54290d net_processing: drop Misbehavior for unconnecting headers (Pieter Wuille)
9f66ac7cf1 net_processing: do not treat non-connecting headers as response (Pieter Wuille)
Pull request description:
So far, discouragement of peers triggers when their misbehavior score exceeds 100 points. Most types of misbehavior increment the score by 100, triggering immediate discouragement, but some types do not. This PR makes all increments equal to either 100 (meaning any misbehavior will immediately cause disconnection and discouragement) or 0 (making the behavior effectively unconditionally allowed), and then removes the logic for score accumulation.
This simplifies the code a bit, but also makes protocol expectations clearer: if a peer misbehaves, they get disconnected. There is no good reason why certain types of protocol violations should be permitted 4 times (howmuch=20) or 9 times (howmuch=10), while many others are never allowed. Furthermore, the distinction between these looks arbitrary.
The specific types of misbehavior that are changed to 100 are:
* Sending us a `block` which does not connect to our header tree (which necessarily must have been unsollicited). [used to be score 10]
* Sending us a `headers` with a non-continuous headers sequence. [used to be score 20]
* Sending us more than 1000 addresses in a single `addr` or `addrv2` message [used to be score 20]
* Sending us more than 50000 invs in a single `inv` message [used to be score 20]
* Sending us more than 2000 headers in a single `headers` message [used to be score 20]
The specific types of misbehavior that are changed to 0 are:
* Sending us 10 (*) separate BIP130 headers announcements that do not connect to our block tree [used to be score 20]
* Sending us more than 8 headers in a single `headers` message (which thus does not get treated as a BIP130 announcement) that does not connect to our block tree. [used to be score 10]
I believe that none of these behaviors are unavoidable, except for the one marked (*) which can in theory happen still due to interaction between BIP130 and variations in system clocks (the max 2 hour in the future rule). This one has been removed entirely. In order to remove the impact of the bug it was designed to deal with, without relying on misbehavior, a separate improvement is included that makes `getheaders`-tracking more accurate.
In another unrelated improvement, this also gets rid of the 8 header limit heuristic to determine whether an incoming non-connecting `headers` is a potential BIP130 announcement, as this rule is no longer needed to prevent spurious Misbehavior. Instead, any non-connecting `headers` is now treated as a potential announcement.
ACKs for top commit:
sr-gi:
ACK [6eecba4](6eecba475e)
achow101:
ACK 6eecba475e
mzumsande:
Code Review ACK 6eecba475e
glozow:
light code review / concept ACK 6eecba475e
Tree-SHA512: e11e8a652c4ec048d8961086110a3594feefbb821e13f45c14ef81016377be0db44b5311751ef635d6e026def1960aff33f644e78ece11cfb54f2b7daa96f946
94ed4fbf8e Add release note for size 2 package rbf (Greg Sanders)
afd52d8e63 doc: update package RBF comment (Greg Sanders)
6e3c4394cf mempool: Improve logging of replaced transactions (Greg Sanders)
d3466e4cc5 CheckPackageMempoolAcceptResult: Check package rbf invariants (Greg Sanders)
316d7b63c9 Fuzz: pass mempool to CheckPackageMempoolAcceptResult (Greg Sanders)
4d15bcf448 [test] package rbf (glozow)
dc21f61c72 [policy] package rbf (Suhas Daftuar)
5da3967815 PackageV3Checks: Relax assumptions (Greg Sanders)
Pull request description:
Allows any 2 transaction package with no in-mempool ancestors to do package RBF when directly conflicting with other mempool clusters of size two or less.
Proposed validation steps:
1) If the transaction package is of size 1, legacy rbf rules apply.
2) Otherwise the transaction package consists of a (parent, child) pair with no other in-mempool ancestors (or descendants, obviously), so it is also going to create a cluster of size 2. If larger, fail.
3) The package rbf may not evict more than 100 transactions from the mempool(bip125 rule 5)
4) The package is a single chunk
5) Every directly conflicted mempool transaction is connected to at most 1 other in-mempool transaction (ie the cluster size of the conflict is at most 2).
6) Diagram check: We ensure that the replacement is strictly superior, improving the mempool
7) The total fee of the package, minus the total fee of what is being evicted, is at least the minrelayfee * size of the package (equivalent to bip125 rule 3 and 4)
Post-cluster mempool this will likely be expanded to general package rbf, but this is what we can safely support today.
ACKs for top commit:
achow101:
ACK 94ed4fbf8e
glozow:
reACK 94ed4fbf8e via range-diff
ismaelsadeeq:
re-ACK 94ed4fbf8e
theStack:
Code-review ACK 94ed4fbf8e
murchandamus:
utACK 94ed4fbf8e
Tree-SHA512: 9bd383e695964f362f147482bbf73b1e77c4d792bda2e91d7f30d74b3540a09146a5528baf86854a113005581e8c75f04737302517b7d5124296bd7a151e3992
881724d443 test: Added test coverage to listsinceblock rpc (kevkevinpal)
Pull request description:
This change is meant to add test coverage to this rpc error https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/transactions.cpp#L666C53-L666C79
This is done by renaming the first block in the blocks folder
---
Doing a quick grep for the error code in our functional tests leads to zero results
`grep -nri "Can't read block from disk" ./test/functional/`
ACKs for top commit:
achow101:
ACK 881724d443
tdb3:
re ACK for 881724d443
rkrux:
tACK [881724](881724d443)
Tree-SHA512: c5dff20cf014d0181f49d6b161f1364e1c6b79e8661047f77f07e21e59f4d1f2fd6f745538c8fc5bd6d4244650a840dd64d184634366f7c21fa67141a60af44a
5cf0a1f230 test: add `createmultisig` P2MS encoding test for all n (1..20) (Sebastian Falbesoner)
0570d2c204 test: add unit test for `keys_to_multisig_script` (Sebastian Falbesoner)
0c41fc3fa5 test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16 (Sebastian Falbesoner)
Pull request description:
While reviewing #28307, I noticed that the test framework's `key_to_multisig_script` helper (introduced in #23305) is broken for pubkey count (n) and threshold (k) values larger than 16. This is due to the implementation currently enforcing a direct single-byte data push (using `CScriptOp.encode_op_n`), which obviously fails for values 17+. Fix that by passing the numbers directly to the CScript list, where it's automatically converted to minimally-encoded pushes (see class method `CScript.__coerce_instance`, branch `isinstance(other, int)`).
The second commit adds a unit test to ensure that the encoding is correct.
ACKs for top commit:
achow101:
ACK 5cf0a1f230
tdb3:
ACK 5cf0a1f230
rkrux:
reACK [5cf0a1f](5cf0a1f230)
Tree-SHA512: 4168a165c3f483ec8e37a27dba1628a7ea0063545a2b7e74d9e20d753fddd7e33d37e1a190434fa6dca39adf9eef5d0211f7a0c1c7b44979f0a3bb350e267562
Prior to this commit, TestEncryptedP2PState would always
send initial_v2_handshake bytes in 2 parts (as required
by early key response test).
For generalising this test and having different v2 handshake
behaviour based on the test type, special behaviours like
sending initial_v2_handshake bytes in 2 parts are executed
only if test_type is set to EARLY_KEY_RESPONSE.
Adds argument --resultsfile to test_runner.py.
Writes comma-separated functional test name, status,
and duration to the file provided with the argument.
Also fixes minor typo in test_runner.py
f58beabe75 test: bumpfee with user specified fee_rate ignores walletIncrementalRelayFee (ismaelsadeeq)
436e88f433 bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee rate (ismaelsadeeq)
Pull request description:
Fixes#26973
When using the `bumpfee` RPC and manually specifying `fee_rate`, there should be no requirement that the new fee must be at least the sum of the original fee and `incrementalFee` (maximum of `relayIncrementalFee` and `WALLET_INCREMENTAL_RELAY_FEE`).
This restriction should only apply when user did not specify `fee_rate`.
> because the GUI doesn't let the user specify the new fee rate yet (https://github.com/bitcoin-core/gui/issues/647), it would be very annoying to have to bump 20 times to increment by 20 sat/vbyte.
The restriction should instead be the new fee must be at least the sum of the original fee and `incrementalFee` (`relayIncrementalFee`)
ACKs for top commit:
achow101:
ACK f58beabe75
murchandamus:
ACK f58beabe75
Tree-SHA512: 193259f87173b7d5a8e68e0e29f2ca7e75c550e3cf0dee3d6d822b5b1e07c2e6dec0bfc8fb435855736ebced97a10dbdbfef72e8c5abde06fdefcba122f2e7f1
429ec1aaaa refactor: Rename CTransaction::nVersion to version (Ava Chow)
27e70f1f5b consensus: Store transaction nVersion as uint32_t (Ava Chow)
Pull request description:
Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.
Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid future potential issues with signedness of this value.
I believe that this is safe and does not actually change what transactions would or would not be considered both standard and consensus valid. Within consensus, the only use of the version in consensus is in BIP68 validation which was already casting it to uint32_t. Within policy, although it is used as a signed int for the transaction version number check, I do not think that this change would change standardness. Standard transactions are limited to the range [1, 2]. Negative numbers would have fallen under the < 1 condition, but by making it unsigned, they are still non-standard under the > 2 condition.
Unsigned and signed ints are serialized and unserialized the same way so there is no change in serialization.
ACKs for top commit:
maflcko:
ACK 429ec1aaaa 🐿
glozow:
ACK 429ec1aaaa
shaavan:
ACK 429ec1aaaa💯
Tree-SHA512: 0bcd92a245d7d16c3665d2d4e815a4ef28207ad4a1fb46c6f0203cdafeab1b82c4e95e4bdce7805d80a4f4a46074f6542abad708e970550d38a00d759e3dcef1
24bc46c83b cli: Add warning for duplicate port definition (tdb3)
e208fb5d3b cli: Sanitize ports in rpcconnect and rpcport (tdb3)
Pull request description:
Adds invalid port detection to bitcoin-cli for -rpcconnect and -rpcport.
In addition to detecting malformed/invalid ports (e.g. those outside of the 16-bit port range, not numbers, etc.), bitcoin-cli also now considers usage of port 0 to be invalid. bitcoin-cli previously considered port 0 to be valid and attempted to use it to reach bitcoind.
Functional tests were added for invalid port detection as well as port prioritization.
Additionally, a warning is provided when a port is specified in both -rpcconnect and -rpcport.
This PR is an alternate approach to PR #27820 (e.g. SplitHostPort is unmodified).
Considered an alternative to 127.0.0.1 being specified in functional tests, but at first glance, this might need an update to test_framework/util.py (e.g. rpc_url), which might be left to a future PR.
ACKs for top commit:
S3RK:
light code review ACK 24bc46c83b
achow101:
ACK 24bc46c83b
cbergqvist:
re ACK 24bc46c83b
Tree-SHA512: c83ab6a30a08dd1ac8b368a7dcc2b4f23170f0b61dd67ffcad7bcda05096d333bcb9821fba11018151f55b2929c0a333bfec15b8bb863d83f41fc1974c6efca5
Not every pseudorandom hash result is a valid x-only public key,
so the pubkey tweaking in the course of creating the output public
key would fail about every second time.
Fix this by treating the hash result as private key and calculate
the x-only public key out of that, to be used then as internal key.
ab98e6fd03 test: add coverage for errors for `combinerawtransaction` RPC (brunoerg)
Pull request description:
This PR adds test coverage for the following errors for the `combinerawtransaction` RPC:
* Tx decode failed
* Missing transactions
* Input not found or already spent
For reference: https://maflcko.github.io/b-c-cov/total.coverage/src/rpc/rawtransaction.cpp.gcov.html
ACKs for top commit:
maflcko:
lgtm ACK ab98e6fd03
tdb3:
ACK ab98e6fd03
Tree-SHA512: 8a133c25dad2e1b236e0278a88796f60f763e3fd6fbbc080f926bb23f9dcc55599aa242d6e0c4ec15a179d9ded10a1f17ee5b6063719107ea84e6099f10416b2
0000276b31 test: Remove redundant verack check (MarcoFalke)
Pull request description:
Currently the sync in `connect_nodes` mentions the `version` and `verack` message types, but only checks the `verack`. Neither check is required, as the `pong` check implies both. In case of failure, the debug log will have to be consulted anyway, so the redundant check doesn't add value.
Also clarify in the comments that the goal is to check the flag `fSuccessfullyConnected` indirectly.
ACKs for top commit:
furszy:
utACK 0000276b31
brunoerg:
ACK 0000276b31
tdb3:
ACK 0000276b31
Tree-SHA512: f9ddcb1436d2f70da462a8dd470ecfc90a534dd6507c23877ef7626e7c02326c077001a42ad0171a87fba5c5275d1970d8c5e5d82c56c8412de944856fdfd6db
This commit fixes a dormant bug in MiniWallet that exists since
support for P2TR was initially added in #23371 (see commit
041abfebe4).
In the course of spending the output, the leaf version byte of the
control block in the witness stack doesn't set the parity bit, i.e.
we were so far just lucky that the used combinations of relevant
data (internal pubkey, leaf script / version) didn't result in a
tweaked pubkey with odd y-parity. If that was the case, we'd get the
following validation error:
`mandatory-script-verify-flag-failed (Witness program hash mismatch) (-26)`
Since MiniWallets can now optionally be tagged (#29939), resulting
in different internal pubkeys, the issue is more prevalent now.
Fix it by passing the parity bit, as specified in BIP341.
Rather than only returning the internal key from the P2TR anyone-can-spend
address creation routine, provide the whole TaprootInfo object, which in turn
contains a dictionary of TaprootLeafInfo object for named leaves.
This data is used in MiniWallet for the default ADDRESS_OP_TRUE mode, in order
to deduplicate the witness script and leaf version of the control block.
39d135e79f test: MiniWallet: respect fee_rate for target_weight, use in mempool_limit.py (Sebastian Falbesoner)
b2f0a9f8b0 test: add framework functional test for MiniWallet's tx padding (Sebastian Falbesoner)
c17550bc3a test: MiniWallet: fix tx padding (`target_weight`) for large sizes, improve accuracy (Sebastian Falbesoner)
Pull request description:
MiniWallet allows to create padded transactions that are equal or slightly above a certain `target_weight` (first introduced in PR #25379, commit 1d6b438ef0), which can be useful especially for mempool-related tests, e.g. for policy limit checks or scenarios to trigger mempool eviction. Currently the `target_weight` parameter doesn't play together with `fee_rate` though, as the fee calculation is incorrectly based on the tx vsize before the padding output is added, so the fee-rate is consequently far off. This means users are forced to pass an absolute fee, which can be quite inconvenient and leads to lots of duplicated "calculate absolute fee from fee-rate and vsize" code with the pattern `fee = (feerate / 1000) * (weight // 4)` on the call-sites.
This PR first improves the tx padding itself to be more accurate, adds a functional test for it, and fixes the `fee_rate` treatment for the `{create,send}_self_transfer` methods. (Next step would be to enable this also for the `_self_transfer_multi` methods, but those currently don't even offer a `fee_rate` parameter). Finally, the ability to pass both `target_weight` and `fee_rate` is used in the `mempool_limit.py` functional test. There might be more use-cases in other tests, that could be done in a follow-up.
ACKs for top commit:
rkrux:
tACK [39d135e](39d135e79f)
ismaelsadeeq:
Code Review ACK 39d135e79f🚀
glozow:
light review ACK 39d135e79f
Tree-SHA512: 6bf8e853a921576d463291d619cdfd6a7e74cf92f61933a563800ac0b3c023a06569b581243166906f56b3c5e8858fec2d8a6910d55899e904221f847eb0953d
f68cba29b3 blockman: Replace m_reindexing with m_blockfiles_indexed (Ryan Ofsky)
1b1c6dcca0 test: Add functional test for continuing a reindex (TheCharlatan)
201c1a9282 indexes: Don't wipe indexes again when already reindexing (TheCharlatan)
804f09dfa1 kernel: Add less confusing reindex options (Ryan Ofsky)
e172553223 validation: Remove needs_init from LoadBlockIndex (TheCharlatan)
533eab7d67 bugfix: Streamline setting reindex option (TheCharlatan)
Pull request description:
When restarting `bitcoind` during an ongoing reindex without setting the `-reindex` flag again, the block and coins db is left intact, but any data from the optional indexes is discarded. While not a bug per se, wiping the data again is
wasteful, both in terms of having to write it again, as well as potentially leading to longer startup times. So keep the index data instead when continuing a prior reindex.
Also includes a bugfix and smaller code cleanups around the reindexing code. The bug was introduced in b47bd95920: "kernel: De-globalize fReindex".
ACKs for top commit:
stickies-v:
ACK f68cba29b3
fjahr:
Code review ACK f68cba29b3
furszy:
Code review ACK f68cba29b3
ryanofsky:
Code review ACK f68cba29b3. Only changes since last review were cherry-picking suggested commits that rename variables, improving comments, and making some tweaks to test code.
Tree-SHA512: b252228cc76e9f1eaac56d5bd9e4eac23408e0fc04aeffd97a85417f046229364673ee1ca7410b9b6e7b692b03f13ece17c42a10176da0d7e975a8915deb98ca
