94d0c418c51223f67e50cfa7fa4f4d838f733914 build: remove already tested headers from AC_CHECK_HEADERS (fanquake)
Pull request description:
These headers are already included in a default set which are checked early during configure.
We already use at least sys/types.h and unistd.h unconditionally in configure.
ACKs for top commit:
TheCharlatan:
Code review ACK 94d0c418c51223f67e50cfa7fa4f4d838f733914
Tree-SHA512: d81ce01d46ef1c226b19a4b906e5b622baeee62f04d75eebd02094585ad4712a6c830314227d3a1c03a5f76be5c659b456bb065c175e3cf391e0ece37fd99ead
f9ce0eadf4eb58d1e2207c27fabe69a5642482e7 For feebump, ignore abandoned descendant spends (John Moffett)
Pull request description:
Closes#26667
To be eligible for fee-bumping, a transaction must not have any of its outputs (eg - change) spent in other unconfirmed transactions in the wallet. This behavior is currently [enforced](9e229a542f/src/wallet/feebumper.cpp (L25-L28)) and [tested](9e229a542f/test/functional/wallet_bumpfee.py (L270-L286)).
However, this check shouldn't apply to spends in abandoned descendant transactions, as explained by #26667.
`CWallet::IsSpent` already carves out an exception for abandoned transactions, so we can just use that.
I've also added a new test to cover this case.
ACKs for top commit:
Sjors:
re-utACK f9ce0eadf4eb58d1e2207c27fabe69a5642482e7
achow101:
ACK f9ce0eadf4eb58d1e2207c27fabe69a5642482e7
furszy:
ACK f9ce0ead
Tree-SHA512: 19d957d1cf6747668bb114e27a305027bfca5a9bed2b1d9cc9e1b0bd4666486c7c4b60b045a7fe677eb9734d746f5de76390781fb1e9e0bceb4a46d20acd1749
04528054fcde61aa00e009dbbe1ac350ca1cf748 [bench] BlockAssembler with mempool packages (glozow)
6ce265acf4ff6ee5057b46bcb8b55abc4422e6f8 [test util] lock cs_main before pool.cs in PopulateMempool (glozow)
8791410662ce3ab7ba6bbe9813c55369edd6e4c9 [test util] randomize fee in PopulateMempool (glozow)
cba5934eb697aedbe1966ebc2817ab87232a1b59 [miner] allow bypassing TestBlockValidity (glozow)
c0588523083c9c78770b8b19a52a919db56250d9 [refactor] parameterize BlockAssembler::Options in PrepareBlock (glozow)
a2de971ba1c588488dde653a76853666429d4911 [refactor] add helper to apply ArgsManager to BlockAssembler::Options (glozow)
Pull request description:
Performance of block template building matters as miners likely want to be able to start mining on a block with transactions asap after a block is found. We would want to know if a mempool PR accidentally caused, for example, a 100x slowdown. An `AssembleBlock()` bench exists, but it operates on a mempool with 101 transactions, each with 0 ancestors or descendants and with the same fee. Adding a bench with a more complex mempool is useful because (1) it's more realistic (2) updating packages can potentially cause the algorithm to take a long time.
ACKs for top commit:
kevkevinpal:
Tested ACK [0452805](04528054fc)
achow101:
ACK 04528054fcde61aa00e009dbbe1ac350ca1cf748
stickies-v:
ACK 04528054f
Tree-SHA512: 38c138d6a75616651f9b1faf4e3a1cd833437a486f4e84308fbee958e8462bb570582c88f7ba7ab99d80191e97855ac2cf27c43cc21585d3e4b0e227effe2fb5
45553e11c965db218733f9ad32ecde391b393443 refactor: Make `ThreadHTTP` return void (Hennadii Stepanov)
Pull request description:
The `bool` return value was introduced in 755aa05174e06effd758eeb78c5af9fb465e9611 (https://github.com/bitcoin/bitcoin/pull/8421).
It has been not used since 8d3f46ec3938e2ba17654fecacd1d2629f9915fd (https://github.com/bitcoin/bitcoin/pull/14670).
No behavior change.
ACKs for top commit:
achow101:
ACK 45553e11c965db218733f9ad32ecde391b393443
brunoerg:
crACK 45553e11c965db218733f9ad32ecde391b393443
w0xlt:
ACK 45553e11c9
stickies-v:
ACK 45553e11c
Tree-SHA512: 1593a5740e729967fbe1363235cd5b77ecf431b29bc740a89a6c70fc838ad97a2e4a2cd7cd63aa482f7c50bc2ffabc8cd53e8f64d6032603cb3b662229bc3dc2
0f5fc4f656d0990802ab552c0e620f49e785fed4 doc: fix up -netinfo relaytxes help (Jon Atack)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/pull/26109#discussion_r995502563 by Marco Falke (thanks!)
ACKs for top commit:
mzumsande:
Code Review ACK 0f5fc4f656d0990802ab552c0e620f49e785fed4
Tree-SHA512: d7345d1a94b15c4ec1a2bb0be5c04c472411d90cefb4c16ed524933d2bfc36816bb7519c2e109b2e41ff451b039dd2ddaa6d5db917ad54745332f2a1d8b85570
9567bfeab95cc0932073641dd162903850987d43 clang-tidy: Add `performance-no-automatic-move` check (Hennadii Stepanov)
Pull request description:
Split from bitcoin/bitcoin#26642 as [requested](https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1054673201).
For the problem description see https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html.
The following types are affected:
- `std::pair<CAddress, NodeSeconds>`
- `std::vector<CAddress>`
- `UniValue`, also see bitcoin/bitcoin#25429
- `QColor`
- `CBlock`
- `MempoolAcceptResult`
- `std::shared_ptr<CWallet>`
- `std::optional<SelectionResult>`
- `CTransactionRef`, which is `std::shared_ptr<const CTransaction>`
ACKs for top commit:
andrewtoth:
ACK 9567bfeab95cc0932073641dd162903850987d43
aureleoules:
ACK 9567bfeab95cc0932073641dd162903850987d43
Tree-SHA512: 9b6a5d539205b41d2c86402d384318ed2e1d89e66333ebd200a48fd7df3ce6f6c60a3e989eda5cc503fb34b8d82526f95e56776e1af51e63b49e3a1fef72dbcb
c467cfffcebb30f829eeb8160166a6b941d97ed6 test: add coverage for `purpose` arg in `listlabels` (brunoerg)
Pull request description:
This PR adds test coverage for `listlabels` command when specifying the `purpose` (send and receive).
dcdfd72861/src/wallet/rpc/addresses.cpp (L698-L704)
ACKs for top commit:
kristapsk:
ACK c467cfffcebb30f829eeb8160166a6b941d97ed6
Tree-SHA512: 7e7143c1264692f7b22952e7c70dbe9ed3f5dcd2e3b69962a47be9f9c21b3f4a9089ca87962fbc8ff9116e7d2dbeb7f36d6a132c9ac13724a255cfe1b32373a8
264f9ef17f650035882d24083fb419845942a3ac [validation] return MempoolAcceptResult for every tx on PCKG_TX failure (glozow)
dae81e01e8698e04afb0db4d1442659c3b48fcf5 [refactor] rename variables in AcceptPackage for clarity (glozow)
da484bc738eca47936a31a2e5ad663417e19f311 [doc] release note effective-feerate and effective-includes RPC results (glozow)
5eab397b9840de5a4729bea723794b529e5fcbb4 [validation] remove PackageMempoolAcceptResult::m_package_feerate (glozow)
601bac88cb95404e7d38ac6348d959c0e06bd922 [rpc] return effective-includes in testmempoolaccept and submitpackage (glozow)
1691eaa818f7a7b22907f756490b842d80a9a21d [rpc] return effective-feerate in testmempoolaccept and submitpackage (glozow)
d6c7b78ef2924af72f677ce2a7472c2447141e18 [validation] return wtxids of other transactions whose fees were used (glozow)
1605886380e4d3ff2e1236739fb800fa07322c49 [validation] return effective feerate from mempool validation (glozow)
5d35b4a7dee95ae70bfdcbe79bc39fe488641b23 [test] package validation quits early due to non-policy, non-missing-inputs failure (glozow)
be2e4d94e59510e0a98408313feb27e97321b16e [validation] when quitting early in AcceptPackage, set package_state and tx result (glozow)
Pull request description:
This PR fixes a bug and improves the mempool accept interface to return information more predictably.
Bug: In package validation, we first try the transactions individually (see doc/policy/packages.md for more explanation) and, if they all failed for missing inputs and policy-related (i.e. fee) reasons, we'll try package validation. Otherwise, we'll just "quit early" since, for example, if a transaction had an invalid signature, adding a child will not help make it valid. Currently, when we quit early, we're not setting the `package_state` to be invalid, so the caller might think it succeeded. Also, we're returning no results - it makes more sense to return the individual transaction failure. Thanks instagibbs for catching https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1013293248!
Also, make the package results interface generally more useful/predictable:
- Always return the feerate at which a transaction was considered for `CheckFeeRate` in `MempoolAcceptResult::m_effective_feerate` when it was successful. This can replace the current `PackageMempoolAcceptResult::m_package_feerate`, which only sometimes exists.
- Always provide an entry for every transaction in `PackageMempoolAcceptResult::m_tx_results` when the error is `PCKG_TX`.
ACKs for top commit:
instagibbs:
reACK 264f9ef17f
achow101:
ACK 264f9ef17f650035882d24083fb419845942a3ac
naumenkogs:
reACK 264f9ef17f650035882d24083fb419845942a3ac
Tree-SHA512: ce7fd9927a80030317cc6157822596e85a540feff5dbf5eea7c62da2eb50c917cdddc9da1e2ff62cc18b98b27d360151811546bd9d498859679a04bbee090837
DOCKER in names is confusingly used as synonym for "image", "container",
and "ci". Fix the confusion by picking the term that fits the context.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:$1:$2:g" $( git grep -l "$1" ) ; }
ren DOCKER_PACKAGES CI_BASE_PACKAGES
# This better reflects that they are the common base for all CI
# containers.
ren DOCKER_ID CI_CONTAINER_ID
# This is according to the documentation of "--detach , -d: Run
# container in background and print container ID".
ren DOCKER_NAME_TAG CI_IMAGE_NAME_TAG
# This avoids confusing with CONTAINER_NAME and clarifies that it is an
# image.
ren DOCKER_ADMIN CI_CONTAINER_CAP
# This clarifies that it is a capability added to the container.
ren DOCKER_CI_CMD_PREFIX CI_EXEC_CMD_PREFIX
# This brings it in line with the CI_EXEC naming.
-END VERIFY SCRIPT-
378400953424598fd78ccec5ba8cc38bc253c550 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow)
Pull request description:
If a wallet has key birthdates that are more recent than the currrent chain tip, or a bestblock height higher than the current tip, we should not attempt to rescan as there is nothing to scan for.
Fixes#26655
ACKs for top commit:
ishaanam:
re-utACK 378400953424598fd78ccec5ba8cc38bc253c550
w0xlt:
utACK 3784009534
furszy:
Code review ACK 37840095
Tree-SHA512: f0d90b62940d97d50f21e1e01fa6dcb54409fad819cea4283612825c4d93d733df323cd92787fed43956b0a8e386a5bf88218f1f5749c913398667a5c8f54470
When an orphaned coinbase is reorged back into the main chain, any
unconfirmed ancestors should still be marked as abandoned due to the
original reorg that orphaned that coinbase.
65e78bda7cd23ad7b6ede63c6bf5ffe6f552c71d test: Invalid label name coverage (Aurèle Oulès)
552b51e682b5a52d9e2fbe64e44e623451692bd3 refactor: Add sanity checks in LabelFromValue (Aurèle Oulès)
67e7ba8e1aea58fc864f9bb1fc0e56b70777185e rpc: Sanitize label name in various RPCs (Aurèle Oulès)
Pull request description:
The following RPCs did not sanitize the optional label name:
- importprivkey
- importaddress
- importpubkey
- importmulti
- importdescriptors
- listsinceblock
Thus is was possible to import an address with a label `*` which should not be possible.
The wildcard label is used for backwards compatibility in the `listtransactions` rpc.
I added test coverage for these RPCs.
ACKs for top commit:
ajtowns:
ACK 65e78bda7cd23ad7b6ede63c6bf5ffe6f552c71d
achow101:
ACK 65e78bda7cd23ad7b6ede63c6bf5ffe6f552c71d
furszy:
diff ACK 65e78bd
stickies-v:
re-ACK 65e78bda7cd23ad7b6ede63c6bf5ffe6f552c71d
theStack:
re-ACK 65e78bda7cd23ad7b6ede63c6bf5ffe6f552c71d
Tree-SHA512: ad99f2824d4cfae352166b76da4ca0069b7c2eccf81aaa0654be25bbb3c6e5d6b005d93960f3f4154155f80e12be2d0cebd5529922ae3d2a36ee4eed82440b31
3076f1815d64f448aa9dff6e48e07004f42ac0fc doc: net: fix link to onion address encoding scheme [ONIONADDRESS] (Sebastian Falbesoner)
Pull request description:
Instead of referring to a fixed line number to a file in master (which is obviously always quickly outdated), use a permalink tied to the latest commit.
ACKs for top commit:
vasild:
ACK 3076f1815d64f448aa9dff6e48e07004f42ac0fc
Tree-SHA512: 7070a7e47d683b1539f33daa4c67093a87d6121a84430a3b24afee139a7f2b3cab32fcdf0bc561f8e177b5ba864a98e4491e08dac90cdd4bd2e4e6b8fa7e4b14
Instead of referring to a fixed line number to a file in master (which
is obviously always quickly outdated), use a permalink tied to the
latest commit.
fae885b98fa1da702d1b36f4bf99209595816984 ci: Run one task with all tests on credits (MarcoFalke)
Pull request description:
This should allow to detect any obvious issues in the tests within 10 minutes of opening a pull request, regardless of the current scheduling load on the Cirrus CI community cluster.
ACKs for top commit:
fanquake:
ACK fae885b98fa1da702d1b36f4bf99209595816984
Tree-SHA512: 384651364648cd38a126f64a5cc650dd396735c05f7d8b5d5c9a8ee7b3b98e36f5dfb3b1c0563ba4d9f554b5e0e82e13aa38e78092f11856a82fc76270fe6f3e
This makes the interface more predictable and useful. The caller
understands one or more transactions failed, and can learn what happened
with each transaction. We already have this information, so we might as
well return it.
It doesn't make sense to do this for other PackageValidationResult
values because:
- PCKG_RESULT_UNSET: this means everything succeeded, so the individual
failures are no longer accurate.
- PCKG_MEMPOOL_ERROR: something went wrong with the mempool logic;
transaction failures might not be meaningful.
- PCKG_POLICY: this means something was wrong with the package as a
whole. The caller should use the PackageValidationState to find the
error, rather than looking at individual MempoolAcceptResults.
This value creates an extremely confusing interface as its existence is
dependent upon implementation details (whether something was submitted
on its own, etc). MempoolAcceptResult::m_effective_feerate is much more
helpful, as it always exists for submitted transactions.
fadfae42f1be240af3198565068b409a62e698a0 ci: Remove unused busybox workaround (MarcoFalke)
fac424fce722fd0a2084d5792ceed200c773158b ci: Create named symbol for BINS_SCRATCH_DIR (MarcoFalke)
Pull request description:
It can be re-added when it is needed again. But it may be more likely that the other workarounds can be removed as well, in a follow-up.
ACKs for top commit:
fanquake:
ACK fadfae42f1be240af3198565068b409a62e698a0
Tree-SHA512: 39984d02ddcba907a5b780fd9cb4dab7f1abbc32468968b4862d226de2e4f95c71708c7361db97347e1ac65196dc40d379e68f34bef37d42b5464d343e7466d8
In certain circumstances, the GUI console will display
the message 'Executing command without any wallet' when
it is, in fact, using the default wallet.
In RPC calls, if no wallet is explicitly selected and
there is exactly one wallet loaded, the default is to
act on that loaded wallet.
The GUI console acts that way in reality, but
erroneously reports that it's not acting on any
particular wallet.
abccb27466c5a240a59f3f7c86f2bc60d51e9fee test: add coverage for absolute timestamp in `setban` (brunoerg)
b99f1f20f773eb55c870a033b3f2e8f13d55d0c8 p2p, rpc: don't allow past absolute timestamp in `setban` (brunoerg)
Pull request description:
We shouldn't allow call `setban` with past absolute timestamp. First, because doesn't make sense to ban a node until ~ past ~. Besides that, it could make an unnecessary write to the DB since `BanMan::Ban` calls `DumpBanlist` and it calls `SweepBanned` which will remove this new ban (because of the timestamp) of the array.
ACKs for top commit:
1440000bytes:
ACK abccb27466
Tree-SHA512: 6d0cadf99fc4f78d77d3bafd6f5c85ac56e243ebc376210fdb2bee751e7b139ec7d6f5f346317fd0b10051b685f2d0ee1d8e40f4bc10f4dbdbbddd5e1ee84de5
fa4e98c77f8c6b693b88c1a6bb811650a6267fdb ci: Fix ci_native_fuzz_msan CONTAINER_NAME (MarcoFalke)
Pull request description:
This avoids a duplicate name with the other msan task, which will lead to errors when running locally:
> Error: creating container storage: the container name "ci_native_msan" is already in use by 77350e26f9c36abbb601140cd0b485ead093ff118803c720ca8b10f6bdfa37d2. You have to remove that container to be able to reuse that name: that name is already in use
ACKs for top commit:
fanquake:
ACK fa4e98c77f8c6b693b88c1a6bb811650a6267fdb
hebasto:
ACK fa4e98c77f8c6b693b88c1a6bb811650a6267fdb, I've verified that there are no other duplicated `CONTAINER_NAME`'s values.
Tree-SHA512: f1b28b21302c0947912d642c12c2ccad236af6824fd27e68341baddedec24087af738f3226028a0eeb6e0fc7e9f90713fc680855eeb07adc113c4f6e8b03a545
Bug: not setting package_state means package_state.IsValid() == true and
the caller does not know that this failed.
We won't be validating this transaction again, so it makes sense to return this
failure to the caller.
Rename package_state to package_state_quit_early to make it more clear
what this variable is used for and what its scope is.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
