Age | Commit message (Collapse) | Author |
|
The current argument name and description are dangerous as it don't
describe the case where the user selects the recipient output as the
change address. This one could end up been increased by the inputs
minus outputs remainder. Which, when bumpfee adds new inputs
to the transaction, leads the process to send more coins to the
recipient. Which is not what the user would expect from a
'reduce_output' param naming.
Co-authored-by: Murch <murch@murch.one>
|
|
encoded tx if complete
a99e9e655a58b2364a74aec5cafb827a73c6b0c4 doc: add release note (ismaelsadeeq)
2b4edf889a4b555c8c7f6793fa5d820e5513ecac test: check `descriptorprocesspsbt` return hex encoded tx (ismaelsadeeq)
c405207a18fdee75a4dea470bb0d13e59e15ce45 rpc: `descriptorprocesspsbt` return hex encoded tx (ismaelsadeeq)
Pull request description:
Coming from [#28414 comment](https://github.com/bitcoin/bitcoin/pull/28414#pullrequestreview-1618684391) Same thing also for `descriptorprocesspsbt`.
Before this PR `descriptorprocesspsbt` returns a boolean `complete` which indicates that the psbt is final, users then have to call `finalizepsbt` to get the hex encoded network transaction.
In this PR if the psbt is complete the return object also has the hex encoded network transaction ready for broadcast with `sendrawtransaction`.
This save users calling `finalizepsbt` with the descriptor, if it is already complete.
ACKs for top commit:
achow101:
ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4
pinheadmz:
ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4
ishaanam:
ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4
Tree-SHA512: c3f1b1391d4df05216c463127cd593f8703840430a99febb54890bc66fadabf9d9530860605f347ec54c1694019173247a0e7a9eb879d3cbb420f9e8d9839b75
|
|
transaction package context
eb8f58f5e4a249d55338304099e8238896d2316d Add functional test to catch too large vsize packages (Greg Sanders)
1a579f9d01256b0b2570168496d129a8b2009b35 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors (Greg Sanders)
bc013fe8e3d0bae2ab766a8248219aa3c9e344df Bugfix: Pass correct virtual size to CheckPackageLimits (Luke Dashjr)
533660c58ad5a218671a9bc1537299b1d67bb55d Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion (Greg Sanders)
Pull request description:
(Alternative) Minimal subset of https://github.com/bitcoin/bitcoin/pull/28345 to:
1) Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT which accounts for additional WU necessary to not exclude default chain limit transactions that would have been accepted individually. Avoids sigops vbyte confusion.
2) pass correct vsize to chain limit evaluations in package context
3) stop overly-large packages that have no existing mempool ancestors (also a bugfix by itself if someone sets non-standard chain limits)
This should fix the known issues while not blocking additional refactoring later.
ACKs for top commit:
achow101:
ACK eb8f58f5e4a249d55338304099e8238896d2316d
ariard:
Re-Code ACK eb8f58f5e
glozow:
reACK eb8f58f5e4a249d55338304099e8238896d2316d
Tree-SHA512: 1b5cca1a526207e25d387fcc29a776a3198c3a013dc2b35c6275b9d5a64db2476c154ebf52e3a1aed0b9924c75613f21a946577aa760de28cadf0c9c7f68dc39
|
|
d05be124dbc0b24fb69d0c28ba2d6b297d243751 test: added coverage to estimatefee (kevkevin)
Pull request description:
Added a assert for an rpc error when we try to estimate fee for the max conf_target
Line I am adding coverage to https://github.com/bitcoin/bitcoin/blob/master/src/rpc/fees.cpp#LL71C52-L71C52
ACKs for top commit:
MarcoFalke:
lgtm ACK d05be124dbc0b24fb69d0c28ba2d6b297d243751
Tree-SHA512: dfab075989446e33d1a5ff1a308f1ba1b9f80cce3848fbe4231f69212ceef456a3f2b19365a42123e0397c31893fd9f1fd9973cc00cfbb324386e12ed0e6bccc
|
|
for invalid command
f52cb02f700b58bca921a7aa24bfeee04760262b doc: make it clear that `node` in `addnode` refers to the node's address (brunoerg)
effd1efefb53c58f0e43fec4f019a19f97795553 test: `addnode` with an invalid command should throw an error (brunoerg)
56b27b84877376ffc32b3bad09f1047b23de4ba1 rpc, refactor: clean-up `addnode` (brunoerg)
Pull request description:
This PR:
- Adds test coverage for an invalid `command` in `addnode`.
- Rename `test_getaddednodeinfo` to `test_addnode_getaddednodeinfo` and its log since this function also tests `addnode` and it doesn't worth to split into 2 ones.
- Makes it clear in docs that `node` in `addnode` refers to the node's address. It seemed a little weird for me "The node (see getpeerinfo for nodes)", it could mean a lot of things e.g. the node id.
- Some small improv/clean-up: use `const` where possible, rename some vars, and remove the check for nullance for `command` since it's a non-optional field.
ACKs for top commit:
achow101:
ACK f52cb02f700b58bca921a7aa24bfeee04760262b
jonatack:
ACK f52cb02f700b58bca921a7aa24bfeee04760262b
theStack:
re-ACK f52cb02f700b58bca921a7aa24bfeee04760262b
Tree-SHA512: e4a69e58b784e233463945b4d55a401957f9fe4562c129f59216a44f44fb3221d3449ac578fb35e665ca654c6ade2e741b72c3df78040f7527229c77b6c5b82e
|
|
signing for tx inputs
83d7cfd5429b0be7755bc48145032956f5f56dae test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs (Sebastian Falbesoner)
Pull request description:
This PR is a simple follow-up for #28025. It introduces a `signing_input_segwitv0` helper in order to deduplicate the following steps needed to create a segwitv0 ECDSA signature:
1. calculate the `SegwitV0SignatureHash` with the desired sighash type
2. create the actual digital signature by calling ECKey.sign_ecdsa on the signature message hash calculated above
3. put the DER-encoded result (plus sighash byte) at the bottom of the witness stack
ACKs for top commit:
achow101:
ACK 83d7cfd5429b0be7755bc48145032956f5f56dae
pinheadmz:
code review ACK at 83d7cfd5429b0be7755bc48145032956f5f56dae
Tree-SHA512: b8e55409ddc9ddb14cfc06daeb4730d7750a4632f175f88dcac6ec4d216e71fd4a7eee325a64d6ebba3b33be50bcd30c2de7400f834c01abb67e52840d9823b6
|
|
|
|
|
|
new/tried table address count
28bac81a346c0b68273fa73af924f7096cb3f41d test: add functional test for getaddrmaninfo (stratospher)
c8eb8dae51039aa1938e7040001a149210e87275 rpc: Introduce getaddrmaninfo for count of addresses stored in new/tried table (stratospher)
Pull request description:
implements https://github.com/bitcoin/bitcoin/issues/26907. split off from #26988 to keep RPC, CLI discussions separate.
This PR introduces a new RPC `getaddrmaninfo`which returns the count of addresses in the new/tried table of a node's addrman broken down by network type. This would be useful for users who want to see the distribution of addresses from different networks across new/tried table in the addrman.
```jsx
$ getaddrmaninfo
Result:
{ (json object) json object with network type as keys
"network" : { (json object) The network (ipv4, ipv6, onion, i2p, cjdns)
"new" : n, (numeric) number of addresses in new table
"tried" : n, (numeric) number of addresses in tried table
"total" : n (numeric) total number of addresses in both new/tried tables from a network
},
...
}
```
### additional context from [original PR](https://github.com/bitcoin/bitcoin/pull/26988)
1. network coverage tests were skipped because there’s a small chance that addresses from different networks could hash to the same bucket and cause count of different network addresses in the tests to fail. see https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1137596851.
2. #26988 uses this RPC in -addrinfo CLI. Slight preference for keeping the RPC hidden since this info will mostly be useful to only super users. see https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173964808.
ACKs for top commit:
0xB10C:
ACK 28bac81a346c0b68273fa73af924f7096cb3f41d
willcl-ark:
reACK 28bac81a346c0b68273fa73af924f7096cb3f41d
achow101:
ACK 28bac81a346c0b68273fa73af924f7096cb3f41d
brunoerg:
reACK 28bac81a346c0b68273fa73af924f7096cb3f41d
theStack:
Code-review ACK 28bac81a346c0b68273fa73af924f7096cb3f41d
Tree-SHA512: 346390167e1ebed7ca5c79328ea452633736aff8b7feefea77460e04d4489059334ae78a3f757f32f5fb7827b309d7186bebab3c3760b3dfb016d564a647371a
|
|
mutating it in package evaluation
ee589d4466bb0548a6f2215afe8abd0735768dab Add regression test for m_limit mutation (Greg Sanders)
275579d8c133c066212a26423639956e2576e97a Remove MemPoolAccept::m_limits, only have local copies for carveouts (Greg Sanders)
Pull request description:
Without remoing it, if we ever call `PreChecks()` multiple times for any reason during any one `MempoolAccept`, subsequent invocations may have incorrect limits, allowing longer/larger chains than should be allowed.
Currently this is only an issue with `submitpackage`, so this is not exposed on mainnet.
ACKs for top commit:
achow101:
ACK ee589d4466bb0548a6f2215afe8abd0735768dab
glozow:
ACK ee589d4466bb0548a6f2215afe8abd0735768dab, nits can be ignored
ariard:
Code Review ACK ee589d446
Tree-SHA512: 14cf8edc73e014220def82563f5fb4192d1c2c111829712abf16340bfbfd9a85e2148d723af6fd4995d503dd67232b48dcf8b1711668d25b5aee5eab1bdb578c
|
|
scripts
8e7e3e614955e60d3bf9e9a481ef8916bf9e22d9 test: wallet, verify migration doesn't crash for an invalid script (furszy)
1de8a2372ab39386e689b27d15c4d029be239319 wallet: disallow migration of invalid or not-watched scripts (furszy)
Pull request description:
Fixing #28057.
The legacy wallet allows to import any raw script (#28126), without
checking if it was valid or not. Appending it to the watch-only set.
This causes a crash in the migration process because we are only
expecting to find valid scripts inside the legacy spkm.
These stored scripts internally map to `ISMINE_NO` (same as if they
weren't stored at all..).
So we need to check for these special case, and take into account that
the legacy spkm could be storing invalid not watched scripts.
Which, in code words, means `IsMineInner()` returning
`IsMineResult::INVALID` for them.
Note:
To verify this, can run the test commit on top of master.
`wallet_migration.py` will crash without the bugfix commit.
ACKs for top commit:
achow101:
ACK 8e7e3e614955e60d3bf9e9a481ef8916bf9e22d9
Tree-SHA512: c2070e8ba78037a8f573b05bf6caa672803188f05429adf5b93f9fc1493faedadecdf018dee9ead27c656710558c849c5da8ca5f6f3bc9c23b3c4275d2fb50c7
|
|
|
|
instead of just scriptPubKey
ad0c469d98c51931b98b7fd937c6ac3eeaed024e wallet: Use CTxDestination in CRecipient rather than scriptPubKey (Andrew Chow)
07d3bdf4ebc06825ea24ab6f7c87aef6a22238c6 Add PubKeyDestination for P2PK scripts (Andrew Chow)
1a98a51c666e9ae77364115775ec2e0ba984e8e0 Allow CNoDestination to represent a raw script (Andrew Chow)
8dd067088d41f021b357d7db5fa5f0a9f61edddc Make WitnessUnknown members private (Andrew Chow)
Pull request description:
For silent payments, we want to provide a `SilentPaymentsDestination` to be used as the recipient, which requires `CRecipient` to use something other than just the `scriptPubKey` as we cannot know the output script for a silent payment prior to transaction creation. `CTxDestination` seems like the obvious place to add a `SilentPaymentsDestination` as it is our internal representation of an address.
In order to still allow paying to arbitrary scriptPubKeys (e.g. for data carrier outputs, or the user hand crafted a raw transaction that they have given to `fundrawtransaction`), `CNoDestination` is changed to contain raw scripts.
Additionally, P2PK scripts are now interpreted as a new `PubKeyDestination` rather than `PKHash`. This results in some things that would have given an address for P2PK scripts to no longer do so. This is arguably more correct.
`ExtractDestination`'s behavior is slightly changed for the above. It now returns `true` for those destinations that have addresses, so P2PK scripts now result in `false`. Even though it returns false for `CNoDestination`, the script will now be included in that `CNoDestination`.
Builds on #28244
ACKs for top commit:
josibake:
ACK https://github.com/bitcoin/bitcoin/pull/28246/commits/ad0c469d98c51931b98b7fd937c6ac3eeaed024e
Tree-SHA512: ef3f8f3c7284779d9806c77c85b21caf910a79a1f7e7f1b51abcc0d7e074f14e00abf30f625a13075e41d94dad6202c10ddff462c0ee74c2ca4aab585b145a52
|
|
|
|
Test that if the processed psbt is complete the hex encoded tx
is returned and remove unneccessary rpc call to finalize the
psbt.
|
|
target feerate
f18f9ef4d31c70e2d71ab90a24511692821418c3 Amend bumpfee for inputs with overlapping ancestry (Murch)
2e35e944dab09eff30952233f8dfc0b12c4553d5 Bump unconfirmed parent txs to target feerate (Murch)
3e3e05241128f68cf12f73ee06ff997395643885 coinselection: Move GetSelectionWaste into SelectionResult (Andrew Chow)
c57889da6650715f3e1153b6104bbdae15fcac90 [node] interface to get bump fees (glozow)
c24851be945b2a633ee44ed3c8a501eee5580b62 Make MiniMinerMempoolEntry fields private (Murch)
ac6030e4d8f7d578cd4a8593f41189efca548064 Remove unused imports (Murch)
d2f90c31ef3b8dee5a3e0804ecc62fa1cfec7cd5 Fix calculation of ancestor set feerates in test (Murch)
a1f7d986e0211e54e21a1d4a570e5f15294dca72 Match tx names to index in miniminer overlap test (Murch)
Pull request description:
Includes some commits to address follow-ups from #27021: https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1554675156
Reduces the effective value of unconfirmed UTXOs by the fees necessary to bump their ancestor transactions to the same feerate.
While the individual UTXOs always account for their full ancestry before coin-selection, we can correct potential overestimates with a second pass where we establish the ancestry and bump fee for the whole input set collectively.
Fixes #9645
Fixes #9864
Fixes #15553
ACKs for top commit:
S3RK:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
ismaelsadeeq:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
achow101:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
brunoerg:
crACK f18f9ef4d31c70e2d71ab90a24511692821418c3
t-bast:
ACK https://github.com/bitcoin/bitcoin/pull/26152/commits/f18f9ef4d31c70e2d71ab90a24511692821418c3, I reviewed the latest changes and run e2e tests against eclair, everything looks good :+1:
Tree-SHA512: b65180c4243b1f9d13c311ada7a1c9f2f055d530d6c533b78c2068b50b8c29ac1321e89e85675b15515760d4f1b653ebd9da77b37c7be52d9bc565a3538f0aa6
|
|
blockstore
de8f9123afbecc3b4f59fa80af8148bc865d0588 test: cover read-only blockstore (Matthew Zipkin)
5c2185b3b624ce87320ec16412f98ab591a5860c ci: enable chattr +i capability inside containers (Matthew Zipkin)
e573f2420244c583e218f51cd0d3a3cac6731003 unit test: add coverage for BlockManager (Matthew Zipkin)
Pull request description:
This PR adds unit and functional tests to cover the behavior described in #2039. In particular, that bitcoind will crash on startup if a reindex is requested but the `blk` files are read-only. Eventually this behavior can be updated with https://github.com/bitcoin/bitcoin/pull/27039. This PR just commits the test coverage from #27039 as suggested in https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1584915782
ACKs for top commit:
jonatack:
ACK de8f9123afbecc3b4f59fa80af8148bc865d0588 modulo suggestions in https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1319010039, tested on macOS, but not on Linux for the Linux-related change in the last push
achow101:
ACK de8f9123afbecc3b4f59fa80af8148bc865d0588
MarcoFalke:
lgtm ACK de8f9123afbecc3b4f59fa80af8148bc865d0588 📶
Tree-SHA512: b9bd684035dcea11c901b649fc39f397a2155a9a8459f3348e67947e387e45312fddeccb52981aef486f8a31deebb5356a7901c1bb94b78f82c24192a369af73
|
|
|
|
Co-authored-by: Andrew Chow <github@achow101.com>
|
|
At the end of coin selection reduce the fees by the difference between
the individual bump fee estimates and the collective bump fee estimate.
|
|
When a transaction uses an unconfirmed input, preceding this commit it
would not consider the feerate of the parent transaction. Given a parent
transaction with a lower ancestor feerate, this resulted in the new
transaction's ancestor feerate undershooting the target feerate.
This commit changes how we calculate the effective value of unconfirmed UTXOs.
The effective value of unconfirmed UTXOs is decreased by the fee
necessary to bump its ancestry to the target feerate. This also impacts
the calculation of the waste metric: since the estimate for the current
fee is increased by the bump fees, unconfirmed UTXOs current fees appear less
favorable compared to their unchanged long term fees.
This has one caveat: if multiple UTXOs have overlapping ancestries, each
of their individual estimates will account for bumping all ancestors.
|
|
evaluation
32c1dd1ad65af0ad4d36a56d2ca32a8481237e68 [test] mempool coins disappearing mid-package evaluation (glozow)
a67f460c3fd1c7eb8070623666d887eefccff0d6 [refactor] split setup in mempool_limit test (glozow)
d08696120e3647b4c2cd0ae8d6e57dea12418b7c [test framework] add ability to spend only confirmed utxos (glozow)
3ea71feb11c261f002ed918f91f3434fd8a23589 [validation] don't LimitMempoolSize in any subpackage submissions (glozow)
d227b7234cd4cfd7c593ffcf8e2f24573d1ebea5 [validation] return correct result when already-in-mempool tx gets evicted (glozow)
9698b81828ff98820fa49c83ca364063233374c6 [refactor] back-fill results in AcceptPackage (glozow)
8ad7ad33929ee846a55a43c55732be0cb8973060 [validation] make PackageMempoolAcceptResult members mutable (glozow)
03b87c11ca0705e1d6147b90da33ce555f9f41c8 [validation] add AcceptSubPackage to delegate Accept* calls and clean up m_view (glozow)
3f01a3dab1c4ee37fd4093b6a0a3b622f53e231d [CCoinsViewMemPool] track non-base coins and allow Reset (glozow)
7d7f7a1189432b1b6245ba25df572229870567cb [policy] check for duplicate txids in package (glozow)
Pull request description:
While we are evaluating a package, we split it into "subpackages" for evaluation (currently subpackages all have size 1 except the last one). If a subpackage has size 1, we may add a tx to mempool and call `LimitMempoolSize()`, which evicts transactions if the mempool gets full. We handle the case where the just-submitted transaction is evicted immediately, but we don't handle the case in which a transaction from a previous subpackage (either just submitted or already in mempool) is evicted. Mainly, since the coins created by the evicted transaction are cached in `m_view`, we don't realize the UTXO has disappeared until `CheckInputsFromMempoolAndCache` asserts that they exist. Also, the returned `PackageMempoolAcceptResult` reports that the transaction is in mempool even though it isn't anymore.
Fix this by not calling `LimitMempoolSize()` until the very end, and editing the results map with "mempool full" if things fall out.
Pointed out by instagibbs in https://github.com/bitcoin/bitcoin/commit/faeed687e5cde5e32750d93818dd1d4add837f24 on top of the v3 PR.
ACKs for top commit:
instagibbs:
reACK https://github.com/bitcoin/bitcoin/pull/28251/commits/32c1dd1ad65af0ad4d36a56d2ca32a8481237e68
Tree-SHA512: 61e7f69db4712e5e5bfa27d037ab66bdd97f1bf60a8d9ffb96adb1f0609af012c810d681102ee5c7baec7b5fe8cb7c304a60c63ccc445d00d86a2b7f0e7ddb90
|
|
Test for scenario(s) outlined in PR 28251.
Test what happens when a package transaction spends a mempool coin which
is fetched and then disappears mid-package evaluation due to eviction or
replacement.
|
|
We want to be able to re-use fill_mempool so that none of the tests
affect each other.
Change the logs from info to debug because they are otherwise repeated
many times in the test output.
|
|
Useful to ensure that the topologies of packages/transactions are as
expected, preventing bugs caused by having unexpected mempool ancestors.
|
|
Duplicates of normal transactions would be found by looking for
conflicting inputs, but this doesn't catch identical empty transactions.
These wouldn't be valid but exiting early is good and AcceptPackage's
result sanity checks assume non-duplicate transactions.
|
|
walletprocesspsbt if complete
2e249b922762f19d6ae61edaad062f31bc2849f3 doc: add release note for PR #28414 (Matthew Zipkin)
4614332fc4514f63fcbe9e6de507f7bb9b7e87e9 test: remove unnecessary finalizepsbt rpc calls (ismaelsadeeq)
e3d484b603abff69c6ebfca5cfb78cf82743d090 wallet rpc: return final tx hex from walletprocesspsbt if complete (Matthew Zipkin)
Pull request description:
See https://github.com/bitcoin/bitcoin/pull/28363#discussion_r1315753887
`walletprocesspsbt` currently returns a base64-encoded PSBT and a boolean indicating if the tx is "complete". If it is complete, the base64 PSBT can be finalized with `finalizepsbt` which returns the hex-encoded transaction suitable for `sendrawtransaction`.
With this patch, `walletprocesspsbt` return object will ALSO include the broadcast-able hex string if the tx is already final. This saves users the extra step of calling `finalizepsbt` assuming they have already inspected and approve the transaction from earlier steps.
ACKs for top commit:
ismaelsadeeq:
re ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
BrandonOdiwuor:
re ACK 2e249b9
Randy808:
Tested ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
achow101:
ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
ishaanam:
ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
Tree-SHA512: 229c1103265a9b4248f080935a7ad5607c3be3f9a096a9ab6554093b2cd8aa8b4d1fa55b1b97d3925ba208dbc3ccba4e4d37c40e1491db0d27ba3d9fe98f931e
|
|
P2PK scripts are not PKHash destinations, they should have their own
type.
This also results in no longer showing a p2pkh address for p2pk outputs.
However for backwards compatibility, ListCoinst will still do this
conversion.
|
|
|
|
(27831 follow-ups)
9f55773a370a0d039e727445ccee6b84e05f562a test: refactor: usdt_mempool: store all events (stickies-v)
bc432704505eb165dd86de39ea3434c6fb7a2514 test: refactor: remove unnecessary nonlocal (stickies-v)
326db63a6819813db55ba0d01ab4fe80f7a0d818 test: log sanity check assertion failures (stickies-v)
f5525ad6808df6afc38e5c6e4767ab577e30629c test: store utxocache events (stickies-v)
f1b99ac94fb77340c4d3a5b4bbc3df28009bc773 test: refactor: deduplicate handle_utxocache_* logic (stickies-v)
ad90ba36bd930f00753643cd1fe0af72d1c828c2 test: refactor: rename inbound to is_inbound (stickies-v)
afc0224cdbe73e326addf5fb98a3e95d941f2104 test: refactor: remove unnecessary blocks_checked counter (stickies-v)
Pull request description:
Various cleanups to the USDT functional tests, largely (but not exclusively) follow-ups to https://github.com/bitcoin/bitcoin/pull/27831#pullrequestreview-1491438045. Except for slightly different logging behaviour in "test: store utxocache events" and "test: log sanity check assertion failures", this is a refactor PR, removing unnecessary code and (imo) making it more readable and maintainable.
The rationale for each change is in the corresponding commit message.
Note: except for "test: store utxocache events" (which relies on its parent, and I separated into two commits because we may want the parent but not the child), all commits are stand-alone and I'm okay with dropping one/multiple commits if they turn out to be controversial or undesired.
ACKs for top commit:
0xB10C:
ACK 9f55773a370a0d039e727445ccee6b84e05f562a. Reviewed the code and ran the USDT interface tests. I stepped through the commits and think all changes are reasonable.
Tree-SHA512: 6c37a0265b6c26d4f9552a056a690b8f86f7304bd33b4419febd8b17369cf6af799cb87c16df35d0c2a1b839ad31de24661d4384eafa88816c2051c522fd3bf5
|
|
`p2p_invalid_block`
3eb03803c4111d09c63550a6a6c09afbe3430bf6 test: remove unused variables in `p2p_invalid_block` (brunoerg)
Pull request description:
ACKs for top commit:
stickies-v:
ACK 3eb03803c4111d09c63550a6a6c09afbe3430bf6
Tree-SHA512: eadae1eb323e5562d1ea0aed43ebf0145f0fdbb6cd6d4646bbf1ca89f384820e7b9cb69f0bb04a949e9f8983a879aee8387d6f7ac3d4e4ea027f8892e656fb98
|
|
sync_with_ping
fae0b21e6c7a27f08ea8f8b49198c734f923b5da test: Combine sync_send_with_ping and sync_with_ping (MarcoFalke)
Pull request description:
This reduces bloat, complexity, and makes tests less fragile to intermittent failures, see https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1315648343.
This should not cause any noticeable slowdown, or may even be faster, because active polling will be done at most once.
ACKs for top commit:
glozow:
Concept ACK fae0b21e6c7a27f08ea8f8b49198c734f923b5da
theStack:
ACK fae0b21e6c7a27f08ea8f8b49198c734f923b5da 🏓
Tree-SHA512: 6c543241a7b85458dc7ff6a6203316b80a6227d83d38427e74f53f4c666a882647a8a221e5259071ee7bb5dfd63476fb03c9b558a1ea546734b14728c3c619ba
|
|
|
|
|
|
|
|
fae405556d56f6f13ce57f69a06b9ec1e825422b scripted-diff: Rename CBlockTreeDB -> BlockTreeDB (MarcoFalke)
faf63039cce40f5cf8dea5a1d24945773c3433a1 Fixup style of moved code (MarcoFalke)
fa65111b99627289fd47dcfaa5197e0f09b8a50e move-only: Move CBlockTreeDB to node/blockstorage (MarcoFalke)
fa8685597e7302fc136f21b6dd3a4b187fa8e251 index: Drop legacy -txindex check (MarcoFalke)
fa69148a0a26c5054dbccdceeac8e117bf449275 scripted-diff: Use blocks_path where possible (MarcoFalke)
Pull request description:
The only reason for the check was to print a warning about an increase in storage use. Now that 22.x is EOL and everyone should have migrated (or decided to not care about storage use), remove the check.
Also, a move-only commit is included. (Rebased from https://github.com/bitcoin/bitcoin/pull/22242)
ACKs for top commit:
TheCharlatan:
ACK fae405556d56f6f13ce57f69a06b9ec1e825422b, though I lack historical context to really judge the second commit fa8685597e7302fc136f21b6dd3a4b187fa8e251.
stickies-v:
ACK fae405556d56f6f13ce57f69a06b9ec1e825422b
Tree-SHA512: 9da8f48767ae52d8e8e21c09a40c949cc0838794f1856cc5f58a91acd3f00a3bca818c8082242b3fdc9ca5badb09059570bb3870850d3807b75a8e23b5222da1
|
|
|
|
feature_config_args
fbcacd4cf0dbe54e51f89cda05ff3db9e378ea12 test: remove fixed timeouts from feature_config_args (Martin Zumsande)
Pull request description:
Fixes #28290
These fixed timeouts aren't affected by the `timeout_factor` option and can therefore cause timeouts in slow environments.
They are also unnecessary for the test because they measure the wrong thing:
While there is an internal waiting time of 60s within `ThreadOpenConnections` (beginning only when that thread is started) for fixed seeds querying, the timeouts here don't measure that but the time from startup until a debug log message is encountered, during which many other things happen in init, so they don't make much sense to me in the first place.
ACKs for top commit:
MarcoFalke:
lgtm ACK fbcacd4cf0dbe54e51f89cda05ff3db9e378ea12
Tree-SHA512: 7bb3b7db2f9666b1929ffb7773c838ee98b0845569428e5d00ecf5234973d534c4f474e213896c71baabd6096a79347bd21b41a17b130053049714eb8a447c79
|
|
responded once per connection
668aa6af8d5fbf047d43cf6f85f3335565637fb9 test: p2p: check that `getaddr` msgs are only responded once per connection (Sebastian Falbesoner)
Pull request description:
This simple PR adds missing test coverage for ignoring repeated `getaddr` requests (introduced in #7856, commit 66b07247a7a9e48e082502338176cc06edf61474):
https://github.com/bitcoin/bitcoin/blob/6f03c45f6bb5a6edaa3051968b6a1ca4f84d2ccb/src/net_processing.cpp#L4642-L4648
ACKs for top commit:
MarcoFalke:
lgtm ACK 668aa6af8d5fbf047d43cf6f85f3335565637fb9
brunoerg:
crACK 668aa6af8d5fbf047d43cf6f85f3335565637fb9
Tree-SHA512: edcdc6501c684fb41911e393f55ded9b044cd2f92918877eca152edd5a4287d1a9d57ae999f1cb42185eae00c3a0af411fcb9bcd5b990ef48849c3834b141584
|
|
delimitation
6e8f6468cbf1320b70cf01333002a31b44cb7c33 removed StrFormatInternalBug quote delimitation (Reese Russell)
Pull request description:
This PR rectifies an unnecessary set of quotes delimiting the contents of ```StrFormatInternalBug```. This is a follow up to MarcoFalke https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1297191493. The method of action was to remove the escaped quotes that were a part of strprintf. A single functional test case was modified to reflect the new output format.
```STR_INTERNAL_BUG``` was applied to https://github.com/bitcoin/bitcoin/pull/28123 in ```std::string RPCArg::ToString(const bool oneline)``` in ```rpc/util.cpp```
The results can be seen below.
Previously

This PR

Additional context can be found here.
https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1271871716
Thank you.
ACKs for top commit:
MarcoFalke:
review ACK 6e8f6468cbf1320b70cf01333002a31b44cb7c33
stickies-v:
ACK 6e8f6468cbf1320b70cf01333002a31b44cb7c33
Tree-SHA512: 35317e31a527630495b566407e37db9941dab7f81cfaeb1ea3309683c48e4273284645ad615f73e646a137b4f2ae35933603e9182a7dbdd22cac98d038c491dc
|
|
|
|
They cannot be scaled by the timeout_factor option and can
therefore cause timeouts in slow environments.
They are also not necessary for the test, since they measure time
frome startup until a debug message is encountered, which
is not restricted to 1 minute by any internal logic within bitcoind.
|
|
category
a3b55c94b9ffde07386aa887149ddca91b364967 [doc] move comment about AlreadyHaveTx DoS score to the right place (glozow)
3b8c17838a561616fd7c933753c7b98b6c6c7c99 [log] add more logs related to orphan handling (glozow)
51b3275cd1de467933f13d8b71286bf5ebd12b4b [log] add category TXPACKAGES for orphanage and package relay (glozow)
a33dde1e41d8923a46db84b50550175fa6149c48 [log] include wtxid in tx {relay,validation,orphanage} logging (glozow)
Pull request description:
This was taken from #28031 (see #27463 for project tracking).
- Log wtxids in addition to txids when possible. This allows us to track the fate of a transaction from inv to mempool accept/reject through logs.
- Additional orphan-related logging to make testing and debugging easier. Suggested in https://github.com/bitcoin/bitcoin/pull/28031#pullrequestreview-1531022386 and https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1269622220
- Add `TXPACKAGES` category for logging.
- Move a nearby comment block that was in the wrong place.
ACKs for top commit:
instagibbs:
reACK https://github.com/bitcoin/bitcoin/pull/28364/commits/a3b55c94b9ffde07386aa887149ddca91b364967
achow101:
ACK a3b55c94b9ffde07386aa887149ddca91b364967
brunoerg:
crACK a3b55c94b9ffde07386aa887149ddca91b364967
mzumsande:
Code review ACK a3b55c94b9ffde07386aa887149ddca91b364967
Tree-SHA512: 21884ef7c2ea2fd006e715574a9dd3e6cbbe8f82d62c6187fe1d39aad5a834051203fda5f355a06ca40c3e2b9561aec50d7c922a662b1edc96f7b552c9f4b24d
|
|
get_previous_releases.py
faf7e69862a56be918bc011afc7f489978301320 test: Support powerpc64le in get_previous_releases.py (MarcoFalke)
Pull request description:
To test: `test/get_previous_releases.py -b -t /tmp/prev_releases v22.0`
On master: `Not sure which binary to download for powerpc64le-unknown-linux-gnu`
Here: (pass)
ACKs for top commit:
fanquake:
ACK faf7e69862a56be918bc011afc7f489978301320
Tree-SHA512: 33d9348f99e0d3924a6a5cba8833ec9e413e80167012b557922f3628069dabd555b02f98a6bfd0eb80e2bbbcdb50865b7bca216e1d080b1546ee4812abda4bc2
|
|
|
|
|
|
in our python linter:
```
./test/lint/lint-python.py:12: DeprecationWarning: pkg_resources is deprecated as an API.
See https://setuptools.pypa.io/en/latest/pkg_resources.html
import pkg_resources
```
The importlib.metadata library was added in Python 3.8, which is currently our
minimum-supported Python version.
For more details about importlib.metadata, see https://docs.python.org/3/library/importlib.metadata.html
|
|
for net-level deadlock situation
b3a93b409e7fb33af77bd34a269a3eae71d3ba83 test: add functional test for deadlock situation (Martin Zumsande)
3557aa4d0ab59c18807661a49070b0e5cbfecde3 test: add basic tests for sendmsgtopeer to rpc_net.py (Martin Zumsande)
a9a1d69391596f57851f545fae12f8851149d2c3 rpc: add test-only sendmsgtopeer rpc (Martin Zumsande)
Pull request description:
This adds a `sendmsgtopeer` rpc (for testing only) that allows a node to send a message (provided in hex) to a peer.
While we would usually use a `p2p` object instead of a node for this in the test framework, that isn't possible in situations where this message needs to trigger an actual interaction of multiple nodes.
Use this rpc to add test coverage for the bug fixed in #27981 (that just got merged):
The test lets two nodes (almost) simultaneously send a single large (4MB) p2p message to each other, which would have caused a deadlock previously (making this test fail), but succeeds now.
As can be seen from the discussion in #27981, it was not easy to reproduce this bug without `sendmsgtopeer`. I would imagine that `sendmsgtopeer` could also be helpful in various other test constellations.
ACKs for top commit:
ajtowns:
ACK b3a93b409e7fb33af77bd34a269a3eae71d3ba83
sipa:
ACK b3a93b409e7fb33af77bd34a269a3eae71d3ba83
achow101:
ACK b3a93b409e7fb33af77bd34a269a3eae71d3ba83
Tree-SHA512: 6e22e72402f3c4dd70cddb9e96ea988444720f7a164031df159fbdd48056c8ac77ac53def045d9208a3ca07437c7c8e34f8b4ebc7066c0a84d81cd53f2f4fa5f
|
|
sources with shallow clone
360ac64b90ee16cc24bd4c574ec7e11760515a79 test: previous releases: speed up fetching sources with shallow clone (Sebastian Falbesoner)
Pull request description:
For the sake of building previous releases, fetching the whole history of the repository for each version seems to be overkill as it takes much more time, bandwidth and disk space than necessary. Create a shallow clone instead with history truncated to the one commit of the version tag, which is directly checked out in the same command. This has the nice side-effect that we can remove the extra `git checkout` step after as it's not needed anymore.
Note that it might look confusing to pass a _tag_ to a parameter named `--branch`, but the git-clone manpage explicitly states that this is supported.
ACKs for top commit:
MarcoFalke:
lgtm ACK 360ac64b90ee16cc24bd4c574ec7e11760515a79
Tree-SHA512: c885a695c1ea90895cf7a785540c24e8ef8d1d9ea78db28143837240586beb6dfb985b8b0b542d2f64e2f0ffdca7c65fc3d55f44b5e1b22cc5535bc044566f86
|
|
failure in "restore using dumped wallet"
c4929cfa50ddb12943198a7f45723eedbd087d8f test: wallet_backup.py, fix intermittent failure in "restore using dumped wallet" (furszy)
Pull request description:
Aiming to fix #25652.
The failure arises because the test expects `init_wallet()` (the test framework function) to create a wallet with no keys. However, the function also imports the deterministic private key used to receive the coinbase coins.
This causes a race within the "restore using dumped wallet" case, where we intend to have a new wallet (with no existing keys) to test the 'importwallet()' RPC result.
The reason why this failure is intermittent is that it depends on other peers delivering the chain right after node2 startup and prior to the test 'node2.getbalance()' call and also the synchronization of the validation queue.
ACKs for top commit:
MarcoFalke:
lgtm ACK c4929cfa50ddb12943198a7f45723eedbd087d8f
Tree-SHA512: 80faa590439305576086a7d6e328f2550c97b218771fc5eba0567feff78732a2605d028a30a368d50944ae3d25fdbd6d321fb97321791a356416f2b790999613
|