c7376cc8d7 tests: Test upgrading wallet with privkeys disabled (Andrew Chow)
3d985d4f43 wallet: Don't generate keys when privkeys disabled when upgrading (Andrew Chow)
Pull request description:
When we're upgrading a wallet, we shouldn't be trying to generate new keys for wallets where private keys are disabled.
Fixes#23610
ACKs for top commit:
laanwj:
Code review ACK c7376cc8d7
benthecarman:
tACK c7376cc8d7 this fixed the issue for me
Tree-SHA512: fa07cf37df9196ff98671bb1ce5c9aa0bab46495066b4dab796d7e8e5d5c7adb414ff56adae4fd3e15658a610995bd19a9e1edb00c46144b0df635c5b343f3a6
fa7991601c Fixup style of VerifyDB (MarcoFalke)
fa462ea787 Avoid implicit-integer-sign-change in VerifyLoadedChainstate (MarcoFalke)
Pull request description:
This happens when checking all blocks (`-1`).
To test:
```
./configure CC=clang CXX=clang++ --with-sanitizers=undefined,integer
make
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./test/functional/rpc_blockchain.py
ACKs for top commit:
theStack:
Code-review ACK fa7991601c
brunoerg:
crACK fa7991601c
Tree-SHA512: bcbe6becf2fbedd21bbde83a544122e79465937346802039532143b2e4165784905a8852c0ccb088b964874df5e5550931fdde3629cbcee3ae237f2f63c43a8e
d41ed32153 p2p: Avoid InitError when downgrading peers.dat (junderw)
Pull request description:
fixes#24188 (also see https://github.com/bitcoin/bitcoin/pull/22762#issuecomment-951063826)
When downgrading, a peers.dat with a future version that has a minimum
required version larger than the downgraded Bitcoin Core version would cause an InitError.
This commit changes this behavior to overwrite the existing peers.dat with
a new empty one.
ACKs for top commit:
prayank23:
reACK d41ed32153
kallewoof:
reACK d41ed32153
Tree-SHA512: c8e625fe36ce0b1aab6c8ef7241c8954038bb856f2de27bdc4814dc9a60e51be28815c7d77d0f96eace49687a0cea02deb713978bbd3a5add742f50a675f2a40
fixes#24188
When downgrading, a peers.dat with a future version that has a minimum
required version larger than the downgraded version would cause an InitError.
This commit changes this behavior to overwrite the existing peers.dat with
a new empty one, while creating a backup in peers.dat.bak.
fad7ddf9e3 test: Run symlink regression tests on Windows (MarcoFalke)
Pull request description:
Seems odd to add tests, but not run them on the platform that needs them most.
ACKs for top commit:
laanwj:
Code review ACK fad7ddf9e3
ryanofsky:
Code review ACK fad7ddf9e3, just removing new test. Would be nice if the test could be added later, of course.
Tree-SHA512: 64b235967a38c2eb90657e8d7a0447bcc8ce81d1b75a275b6c48bd42efd9ea7e7939257e484f297ee84598def3738eaeb289561aeba1dd6a99b258d389995139
Given the low possibility of a reorg reverting the segwit soft fork,
there is no need to check whether segwit is active here. Also,
TestBlockValidity is run on the block template after it has been
created.
5a89bed410 contrib: address gen-manpages feedback from #24263 (fanquake)
2618fb8d15 Output license info when binaries are passed -version (fanquake)
4c3e3c5746 refactor: shift CopyrightHolders() and LicenseInfo() to clientversion.cpp (fanquake)
Pull request description:
Addresses a review comment from #24263, and addresses the [comment](https://github.com/bitcoin/bitcoin/pull/24263#issuecomment-1030582925) where it was pointed out that we are inconsistent with emitting our copyright. After this change, the copyright is always emitted with `-version`, rather than `-help`, i.e:
```bash
bitcoind -version
Bitcoin Core version v22.99.0-fc1f355913f6-dirty
Copyright (C) 2009-2022 The Bitcoin Core developers
Please contribute if you find Bitcoin Core useful. Visit
<https://bitcoincore.org/> for further information about the software.
The source code is available from <https://github.com/bitcoin/bitcoin>.
This is experimental software.
Distributed under the MIT software license, see the accompanying file COPYING
or <https://opensource.org/licenses/MIT>
```
The info is also added to binaries other than `bitcoind`/`bitcoin-qt`. This change also prevents duplicate copyright info appearing in the `bitcoind` man page.
ACKs for top commit:
laanwj:
Tested ACK 5a89bed410
Tree-SHA512: 0ac2a1adf9e9de0c3206f35837008e3f93eaf15b193736203d71609273f0887cca20b8a90972cb9f941ebd62b330d61a0cbb5fb1b7a7f2dbc715ed8a0c1569d9
48742693ac Replace "can not" with "cannot" in docs, user messages, and tests (Jon Atack)
e670edd434 User-facing content fixups from transifex translator feedback (Jon Atack)
Pull request description:
Closes#24366.
ACKs for top commit:
laanwj:
Code review re-ACK 48742693ac
hebasto:
re-ACK 48742693ac, only suggested change since my previous [review](https://github.com/bitcoin/bitcoin/pull/24367#pullrequestreview-885938219).
Tree-SHA512: 4dcdcb417251a413e65fab6070515e13a1267c8e0dbcf521386b842511391f24c84a0c2168fe13458c977682034466509bf2a3453719d4d94d3c568fd9f4adb4
bfcd60f5d5 test: activate all index types in feature_init.py (Martin Zumsande)
0243907fae index: Don't commit without valid m_best_block_index (Martin Zumsande)
Pull request description:
When an index thread receives an interrupt during init before it got to index anything (so `m_best_block_index == nullptr` still), it will still try to commit previous "work" before stopping the thread. That means that `BaseIndex::CommitInternal()` calls `GetLocator(nullptr)`, which returns an locator to the tip ([code](06b6369766/src/chain.cpp (L31-L32))), and saves it to the index DB.
On the next startup, this locator will be read and it will be assumed that we have successfully synced the index to the tip, when in reality we have indexed nothing.
In the case of coinstatsindex, this would lead to a shutdown of bitcoind without any indication what went wrong. For the other indexes, there would be no immediate shutdown, but the index would be corrupt.
This PR fixes this by not committing when `m_best_block_index==nullptr`, and it also adds an error log message to the silent coinstatsindex shutdown path.
This is another small bug found by `feature_init.py` - the second commit enables blockfilterindex and coinstatsindex for this test, enabling coinstatsindex without the first commit would have led to frequent failures.
ACKs for top commit:
fjahr:
reACK bfcd60f5d5
shaavan:
reACK bfcd60f5d5
Tree-SHA512: 8e2bac0fc40cde209518a9e59b597ae0a5a875a2a90898673987c91733718d40e528dada942bf552b58bc021bf46e59da2d0cc5a61045f48f9bae2b1baf6033b
f11dad22a5 test: refactor: remove unneeded bytes<->hex conversions in `byte_to_base58` (Sebastian Falbesoner)
Pull request description:
It seems like the only reason for using hex strings in this method was to have a convenient way to convert to an integer from the input data interpreted as big-endian. In Python3 we have `int.from_bytes(..., 'big')` for that purpose, hence there is no need for that anymore and we can simply operate on bytes only.
ACKs for top commit:
laanwj:
Code review ACK f11dad22a5
Tree-SHA512: 9b1563010066ca74d85139c3b9259e9a5bb49e1f141c30b6506a0445afddb2bde7fd421fdd917dc516956e66f93610e2c21d720817640daee8f57f803be76ee4
It seems like the only reason for using hex strings in this
method was to have a convenient way to convert to an integer
from the input data interpreted as big-endian.
In Python3 we have `int.from_bytes(..., 'big')` for that
purpose, hence there is no need for that anymore and we can
simply operate on bytes only.
fa6065661a refactor: Avoid unsigned integer overflow in core_write (MarcoFalke)
Pull request description:
Also, I find the new code a bit easier to understand.
ACKs for top commit:
shaavan:
Code Review ACK fa6065661a
Tree-SHA512: cd751e3b4dc97ef525eb8be8d0a49e9629389cb114df18d59a06e05388822af2939078e937f01494e6b317d601743b1a433ba47aa40c4dc602372d1f0fd0dc11
b75f4c89ec RPC: Return external_signer in getwalletinfo (Kristaps Kaupe)
Pull request description:
Add `external_signer` to the result object of `getwalletinfo` RPC which indicates whether `WALLET_FLAG_EXTERNAL_SIGNER` flag is set for the wallet.
ACKs for top commit:
S3RK:
utACK b75f4c89ec
achow101:
ACK b75f4c89ec
prayank23:
utACK b75f4c89ec
brunoerg:
utACK b75f4c89ec
Tree-SHA512: 066ccb97541fd4dc3d9728834645db714a3c8c93ccf29142811af4d79cfb9440a97bbb6c845434a909bc6e1775ef3737fcbb368c1f0582bc63973f6deb17a45f
fac62056b5 Fix integer sanitizer suppressions in validation.cpp (MarcoFalke)
Pull request description:
It doesn't seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file.
Fix it with a refactor and remove the suppression.
ACKs for top commit:
hebasto:
ACK fac62056b5, I have reviewed the code and it looks OK, I agree it can be merged.
prayank23:
Code Review ACK fac62056b5
Tree-SHA512: efc5b9887cb2e207033b264ebf425bae5ff013e909701c049aea5d79a21f10495826e962d171b3d412717cbf0a4723e5124133b5401b35a73915212e85e91020
d1fab9d5d2 test: Call ceildiv helper with integer (Martin Zumsande)
Pull request description:
On master,
`assert_fee_amount(Decimal("0.00000993"), 217, Decimal("0.00004531"))` passes
`assert_fee_amount(Decimal("0.00000993"), Decimal("217"), Decimal("0.00004531"))` fails.
the reason is that the // operator in `ceildiv(a,b) = -(-a//b)` has a different behavior for Decimals, see [doc](https://docs.python.org/3/library/decimal.html#decimal-objects).
`wallet_send.py` calls this function with Decimals, and I think this is the reason for the failure reported in the OP of #24151 (`wallet_send.py --legacy-wallet` line 332, the numbers used in the example above are from there). However, the other failures reported there cannot be explained by this, so this is just a partial fix.
ACKs for top commit:
ryanofsky:
Code review ACK d1fab9d5d2. Tracking down this problem was a good find, and code seems safer and easier to understand now
Tree-SHA512: 5bf0568cd1a0824f6b1a15a03580b6e9391b4f51112a97c1d00469d255bf6dda45c49a36fa567a5ba9b9973efe1d9cdd480db91965c9f4c2aa963629a8a32cba
a036358994 test: Repair failfast option for test runner (Martin Zumsande)
Pull request description:
Fixes#23990
After #23799, the `--failfast` option in the test runner for the functional tests stopped working, because a second outer loop was introduced, which would have needed a `break` too for the test runner to fail immediately. This also led to the errors reported in #23990.
This provides a straightforward fix for that.
There is also #23995 which is a larger refactor, but that hasn't been updated in a while to fix the failfast issue.
ACKs for top commit:
pg156:
Tested ACK a036358994. I agree adding the `all_passed` flag to break out of the outer loop when needed makes sense. The "failfast" option works after this change.
Tree-SHA512: 3e2f775e36c13d180d32a05cd1cfe0883274e8615cdbbd4e069a9899e9b9ea1091066cf085e93f1c5326bd8ecc6ff524e0dad7c638f60dfdb169fefcdb26ee52
fad81548fa test: Avoid testing negative block heights (MarcoFalke)
Pull request description:
A negative chain height is only used to denote an empty chain, not the height of any block.
So stop testing that and remove a suppression.
ACKs for top commit:
brunoerg:
crACK fad81548fa
Tree-SHA512: 0f9e91617dfb6ceda99831e6cf4b4bf0d951054957c159b1a05a178ab6090798fae7368edefe12800da24585bcdf7299ec3534f4d3bbf5ce6a6eca74dd3bb766
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
fad84a2595 refactor: Fixup uint64_t-cast style in touched line (MarcoFalke)
fa041878de Fix implicit-integer-sign-change in bloom (MarcoFalke)
Pull request description:
Signed values don't really make sense when using `std::vector::operator[]`.
Fix that and remove the suppression.
ACKs for top commit:
PastaPastaPasta:
utACK fad84a2595
theStack:
Code-review ACK fad84a2595
Tree-SHA512: 7139dd9aa098c41e4af1b6e63dd80e71a92b0a98062d1676b01fe550ffa8e21a5f84a578afa7a536d70dad1b8a5017625e3a9e2dda6f864b452ec77b130ddf2a
438e6f4c33 test: speedup interface_rest.py by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
11b9684dfd test: use MiniWallet for rest_interface.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (interface_rest.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078.
Note that the original test sent funds from one node to another and checked node's balances, but the state of a node's wallet is not relevant to any of the REST endpoints, i.e. the replacement is quite stright-forward. In an additional commit, the test is further sped up by using the good ol' immediate tx relay trick (parameter `-whitelist=noban@127.0.0.1`).
ACKs for top commit:
brunoerg:
ACK 438e6f4c33
Tree-SHA512: eac351c5fb7e043c36c193d51545f20f563be9aaa04f3429a2bfb452ae4aa72294d2552800d6cac55c9a3ec2b4f30bcda2abcd74736dec3ed75e7d83c5af437f
3ee6d0788e test: add more wallet conflicts assertions (S3RK)
3b98bf9c43 Revert "Add to spends only transcations from me" (S3RK)
Pull request description:
This reverts commit d04566415e from #22929.
This commit was based on invalid assumption that `mapTxSpends` should contain only outgoing txs and broke wallet conflicts feature.
ACKs for top commit:
achow101:
ACK 3ee6d0788e
Tree-SHA512: bf5a77ced6bac57d5eb85771d9189c53e1edc295d179ed5a1bdce18e365794a9101b4cecf35387b27f67260db3b47f7214e7876e490494529b748cceeb95632d
fa2406a50a zmq: Fix implicit-integer-sign-change (MarcoFalke)
Pull request description:
uint256::begin() returns unsigned data, so there is no reason to make it signed.
Fix that and remove the sanitizer suppression.
ACKs for top commit:
hebasto:
ACK fa2406a50a
PastaPastaPasta:
utACK fa2406a50a, I have reviewed the code and think it makes sense
Tree-SHA512: 150ebcf3fdc3e0f60b6fd8e5fe638737b01e8a0863296bd545fb5ed17d33ab23b2ff94204996aa7b4617650b7383bd86ed2d2bf46746b410feae449de179a2bd
faa630aa15 test: Fix sanitizer suppresions in streams_tests (MarcoFalke)
Pull request description:
Two changes (that also make sense on their own) to remove the file-wide sanitizer suppression:
* `FindByte` no longer takes a `char`, but an `uint8_t`, after commit 196b459920.
* The `key` vector of unsigned chars can be removed and inlined as initializer-list. This avoids a bunch of verbose code like `clear()` and `push_back` of `char`s.
ACKs for top commit:
PastaPastaPasta:
utACK faa630aa15, I have reviewed the changes and agree it makes sense to merge
Tree-SHA512: 747b9d4676fad6d07f3955668639c93333625e69199ff4c499f01167de3875990d93db85e775a7f5b1b684575dceaec8aa000b4db15525fc47b699bac1c85e3d
By whitelisting the peers via -whitelist, the inventory is transmissioned
immediately rather than on average every 5 seconds, speeding up the test by at
least a factor of two:
before:
$ time ./interface_rest.py
...
0m14.82s real 0m01.44s user 0m01.19s system
with this commit:
$ time ./interface_rest.py
...
0m05.67s real 0m01.07s user 0m01.35s system
fa7b07571f test: Fix feature_init intermittent issues (MarcoFalke)
fa4595deb3 test: Remove random line number feature from feature_init.py (MarcoFalke)
Pull request description:
The test doesn't work currently because the log might be finalized before `wait_for_debug_log` is started, in which case it will assume the log is empty and fail to detect any line.
Fix this by calling `wait_for_debug_log` first. Fixes#24060.
Also, remove the "random line number" part of the test, because it doesn't really test anything novel. `wait_for_debug_log` is inherently racy, so will randomly terminate at the exact point or later. So the randomization is already sufficiently covered.
ACKs for top commit:
jamesob:
ACK fa7b07571f
mzumsande:
Code Review ACK fa7b07571f
Tree-SHA512: f107a04926270a1d9ac8f53b6e5254566d1827ba7d27fd74f9ce909319f8794f8bd46da9c42256bf41b6df26c0bad8b9a0d54e4e148695189af09f31e3948330
This is needed for the next commit.
Also, it doesn't really test anything novel. wait_for_debug_log is
inherently racy, so will randomly terminate at the exact point or later.
So the randomization is already sufficiently covered by the existing
test.
fa832103aa Avoid integer sanitizer warnings in chain.o (MarcoFalke)
Pull request description:
The two changes make the code more self-documenting and also allow to remove 5 file-wide suppressions for the module
ACKs for top commit:
PastaPastaPasta:
utACK fa832103aa
jonatack:
ACK fa832103aa
Tree-SHA512: d32a06099c56eed9f69130a3209f989872acc593f849528acd7746ee6caa96688cc32de37e8e59ad5d25dcb8912e341f1a43e50642dadeff6ca7624d0873ad10
faa75fa193 Avoid unsigned integer overflow in bitcoin-tx (MarcoFalke)
Pull request description:
While `npos` means "largest unsigned value" and adding `1` to it yields `0`, it may be clearer to just assign `0` to it and only increment otherwise.
This also allows to remove a file-wide suppression for `unsigned-integer-overflow`.
ACKs for top commit:
hebasto:
ACK faa75fa193, I have reviewed the code and it looks OK, I agree it can be merged.
theStack:
Code-review ACK faa75fa193
Tree-SHA512: c24436641e5d801341c948b812c7f711d5dff70efdf04af00fd3221f4b81d93f25608dddaa36230ba81ca7ab0d18bdd957095d4561e22621e4d69017934f0a16
a380922891 Release notes for getdeploymentinfo rpc (Anthony Towns)
240cad09ba rpc: getdeploymentinfo: include signalling info (Anthony Towns)
376c0c6dae rpc: getdeploymentinfo: include block hash/height (Anthony Towns)
a7469bcd35 rpc: getdeploymentinfo: change stats to always refer to current period (Anthony Towns)
7f15c1841b rpc: getdeploymentinfo: allow specifying a blockhash other than tip (Anthony Towns)
fd826130a0 rpc: move softfork info from getblockchaininfo to getdeploymentinfo (Anthony Towns)
Pull request description:
The aim of this PR is to improve the ability to monitor soft fork status. It first moves the softfork section from getblockchaininfo into a new RPC named getdeploymentinfo, which is then also able to query the status of forks at an arbitrary block rather than only at the tip. In addition, bip9 status is changed to indicate the status of the given block, rather than just for the next block, and an additional field is included to indicate whether each block in the signalling period signaled.
ACKs for top commit:
laanwj:
Code review and lightly tested ACK a380922891
Sjors:
tACK a380922891
fjahr:
tACK a380922891
Tree-SHA512: 7417d733b47629f229c5128586569909250481a3e94356c52fe67a03fd42cd81745246e384b98c4115fb61587714c879e4bc3e5f5c74407d9f8f6773472a33cb
6498ba151b transaction decoding infer output descriptors (Gregory Sanders)
Pull request description:
Following discussion in #16725 this is complementary data to expose. All outputs are inferred.
ACKs for top commit:
achow101:
ACK 6498ba151b
meshcollider:
utACK 6498ba151b
Tree-SHA512: 36664117ddbe46d5fdde7ed6541ef2c9d8dfb7a3636b97f363bf1c325096fe00d9d2acea2d1917ea19fdb82f1ea296c12e440c5c703d6a9bfc1a02fba028bcd8
449dffc610 test, bugfix: fix intermittent failure in getrawtransaction_tests (Jon Atack)
Pull request description:
Easiest to review with `colorMoved = dimmed-zebra` and `colorMovedWs = allow-indentation-change`.
Resolves#23991
Top commit has no ACKs.
Tree-SHA512: a50c597857e45c52dae1b6fc5988ea077c09f2b32ffe3605f0c64bd780f5dd2635755cb26c4981233bf925c5ea9f7310163349a549239dc85657558f253ffb7b
75656adfd2 test: add functional test for `-maxtipage` parameter (Sebastian Falbesoner)
Pull request description:
This PR adds a missing test for the `-maxtipage` parameter which controls what is the allowed maximum tip age for leaving IBD:
792d0d8d51/src/init.cpp (L540)
Relevant code path in the `CChainState::IsInitialBlockDownload` method:
792d0d8d51/src/validation.cpp (L1479-L1480)
The test is pretty simple and should be self-explanatory.
ACKs for top commit:
MarcoFalke:
review ACK 75656adfd2
Tree-SHA512: 0a10dca13cb18c29e64fc8412f4c8f2bcaff1bab8645bd85266c242ba88ce036a150c03cbbe9810c3bb44649810af0aa9cb3584dbae886a7bdb16b72150d08de
fa3bdbd37b test: remove unused sanitizer suppressions (MarcoFalke)
Pull request description:
Looks like those are not needed (anymore)
ACKs for top commit:
fanquake:
ACK fa3bdbd37b
Tree-SHA512: 4bedb6363aba8ea7763291ee0cd074e6bfd77e691bb32999c3959393864dc396bacba1eced2b10d9d600b66e8b83b91f7bc6692331dbd113bbaa87e72d11e2e8
fac8165443 Remove unused checkFinalTx (MarcoFalke)
fa272eab44 wallet: Avoid dropping confirmed coins (MarcoFalke)
888841ea8d interfaces: Remove unused is_final (MarcoFalke)
dddd05e7a3 qt: Treat unconfirmed txs as unconfirmed (MarcoFalke)
Pull request description:
The wallet has several issues:
## Unconfirmed txs in the GUI
The GUI clumsily attempts to guess if unconfirmed txs are locked until a future time. This is currently based on the locktime only, not nSequence, thus wrong. Fix this by removing the clumsy code and treat all unconfirmed txs as unconfirmed. The GUI already prints whether a tx is in the mempool, in which case the user knows that the tx wasn't locked until a future time. If the tx is not in the mempool, it might be better to report the exact reject reason from the mempool instead of using incorrect heuristics.
## Confirmed txs in the wallet
The wallet drops coins that it incorrectly assumes to be locked until a future time, even if they are already confirmed in the chain. This is because the wallet is using the wrong time (adjusted network time) instead of MTP, due to the `-1` default argument of `CheckFinalTx`.
The issues are fixed in separate commits and there is even a test.
ACKs for top commit:
achow101:
ACK fac8165443
prayank23:
reACK fac8165443
glozow:
code review ACK fac8165443, I understand now how this fixes both issues.
Tree-SHA512: 210afb855f4c6d903fee49eba6b1a9735d699cf0168b669eabb38178e53b3a522258b7cc669f52489c6cd3e38bf358afde12eef3ba2e2f2ffaeb06b8f652ccd0
3866272c45 tests: Test specifying input weights (Andrew Chow)
6fa762a372 rpc, wallet: Allow users to specify input weights (Andrew Chow)
808068e90e wallet: Allow user specified input size to override (Andrew Chow)
4060c50d7e wallet: add input weights to CCoinControl (Andrew Chow)
Pull request description:
When funding a transaction with external inputs, instead of providing solving data, a user may want to just provide the maximum signed size of that input. This is particularly useful in cases where the input is nonstandard as our dummy signer is unable to handle those inputs.
The input weight can be provided to any input regardless of whether it belongs to the wallet and the provided weight will always be used regardless of any calculated input weight. This allows the user to override the calculated input weight which may overestimate in some circumstances due to missing information (e.g. if the private key is not known, a maximum size signature will be used, but the actual signer may be doing additional work which reduces the size of the signature).
For `send` and `walletcreatefundedpsbt`, the input weight is specified in a `weight` field in an input object. For `fundrawtransaction`, a new `input_weights` field is added to the `options` object. This is an array of objects consisting of a txid, vout, and weight.
Closes#23187
ACKs for top commit:
instagibbs:
reACK 3866272c45
glozow:
reACK 3866272 via range-diff
t-bast:
ACK 3866272c45
Tree-SHA512: 2c8b471ee537c62a51389b7c4e86b5ac1c3a223b444195042be8117b3c83e29c0619463610b950cbbd1648d3ed01ecc5bb0b3c4f39640680da9157763b9b9f9f
c5b36b1c1b Mempool Update Cut-Through Optimization (Jeremy Rubin)
c49daf9885 [TESTS] Increase limitancestorcount in tournament RPC test to showcase improved algorithm (Jeremy Rubin)
Pull request description:
Often when we're updating mempool entries we update entries that we ultimately end up removing the updated entries shortly thereafter. This patch makes it so that we filter for such entries a bit earlier in processing, which yields a mild improvement for these cases, and is negligible overhead otherwise.
There's potential for a better -- but more sophisticated -- algorithm that can be used taking advantage of epochs, but I figured it is better to do something that is simple and works first and upgrade it later as the other epoch mempool work proceeds as it makes the patches for the epoch algorithm simpler to understand, so you can consider this as preparatory work. It could either go in now if it is not controversial, or we could wait until the other patch is ready to go.
ACKs for top commit:
instagibbs:
reACK c5b36b1
sipa:
utACK c5b36b1c1b
mzumsande:
Code Review ACK c5b36b1c1b
Tree-SHA512: 78b16864f77a637d8a68a65e23c019a9757d8b2243486728ef601d212ae482f6084cf8e69d810958c356f1803178046e4697207ba40d6d10529ca57de647fae6
Added tests to rpc_fundrawtransaction, wallet_send, and rpc_psbt that
test that external inputs can be spent when input weight is provided.
Also tested that the input weight overrides any calculated weight.
Additionally, rpc_psbt's external inputs test is cleaned up a bit to be
more similar to rpc_fundrawtransaction's and avoid potential pitfalls
due to non-deterministic coin selection behavior.
d8dfc403f7 script: redirecting stderr to stdout before pipelining into grep (anouar kappitou)
30df5c3dd4 script: preventing non-compatible sed binary. (anouar kappitou)
Pull request description:
This Pull request improve scripted diff by checking for non-compatible sed binary. Fixes#19815
ACKs for top commit:
shaavan:
reACK d8dfc403f7
theStack:
Tested ACK d8dfc403f7
Tree-SHA512: 4813abb02195e04b8953662e51cba4599789e6185a6d384f56255e2d37c2b16be46c7c8861e44ba1a13d42573a58eb708901472925cb308dba2f602020e9f0f6
dc5d6b0d47 fs: Make compatible with boost 1.78 (Andrew Chow)
Pull request description:
Boost 1.78 removed `operator+` in a way that breaks our usage of it in a subclass. A [proposed workaround](https://github.com/boostorg/filesystem/issues/223#issuecomment-1000230207) for this is to cast the argument to `boost::filesystem::path`, and this is backwards compatible with older versions of boost.
Additionally, it appears that `fs::canonical` no longer removes trailing slashes. This was causing a test to fail. The solution is to explicitly remove the trailing separator in the one place that `fs::canonical` is used.
Lastly, `fs::create_directories` now has an error message saying `create_directories` instead of `create_directory`. This caused wallet_multiwallet.py to fail. The error message check has been updated to be able accept either string.
Fixes#23846
ACKs for top commit:
ryanofsky:
Code review ACK dc5d6b0d47
vincenzopalazzo:
ACK dc5d6b0d47
Tree-SHA512: d4d8e7b49b8dfbf0ced9bfe9a2b3827841227fc755fc799f19159076b0ccf882432cc8b6ad93cdeda98fb58b942b9ba50a9e0a6b4f6b1e0097e80f1074ae5682
bd52684508 test: rest /tx with an invalid/unknown txid (brunoerg)
Pull request description:
This PR adds test coverage to the endpoint `/tx` (rest) passing an invalid and an unknown txid to test its return.
Invalid -> should return status code 400 (bad request)
Unknown -> should return status code 404 (not found)
ACKs for top commit:
kallewoof:
ACK bd52684508
Tree-SHA512: a7fbb63f30d06fc0855133a36e8317c7930ba13aa2b4a2dd1fc35079d59eacace72e1ffe7ae1b3e067066fe51792415940d72d923e83a659a0d5965e4110b32a
fa99e108e7 Fix implicit-integer-sign-change in arith_uint256 (MarcoFalke)
Pull request description:
This refactor doesn't change behaviour, but clarifies that the numbers being dealt with aren't supposed to be negative. This helps when reading the code and allows to remove a sanitizer suppression for the whole file.
ACKs for top commit:
PastaPastaPasta:
utACK fa99e108e7
shaavan:
ACK fa99e108e7
Tree-SHA512: f227e2fd22021e39f0445ec041f4a299d13477c23cef0fc06c53fb3313cbe550cec329336224a7e8775d9045b8009423052b394e83d42a1e40772085dfcdd471
fa7238300c fuzz: Limit fuzzed time to years 2000-2100 (MarcoFalke)
Pull request description:
It doesn't make sense to fuzz times in the past, as Bitcoin Core will refuse to start in the past.
Fix that and also remove a sanitizer suppression, which would be hit in net_processing in `ProcessMessage`:
```cpp
if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60)
addr.nTime = nNow - 5 * 24 * 60 * 60; // <-- Here
```
This changes the format of fuzz inputs. Previously a time value was (de)serialized as 40 bytes, now it is 32 bytes.
ACKs for top commit:
mzumsande:
Code Review ACK fa7238300c
Tree-SHA512: ca6e7233beec2d9ef9fd481d8f1331942a4d2c8fe518b857629bebcc53a4f42ae123b994cf5d359384a0a8022098ff5a9c146600bc2593c6d88734e25bc240ad
On a period boundary, getdeploymentinfo (and previously getblockchaininfo)
would report the status and statistics for the next block rather than
the current block. Change this to always report the status/statistics
of the current block, but add status-next to report the status for the
next block.
aa8a65e4a8 test: use MiniWallet for mempool_accept.py (Sebastian Falbesoner)
b24f6c6855 test: MiniWallet: support default `from_node` for creating txs (Sebastian Falbesoner)
f30041c914 test: create txs with current `nVersion` (2) by default (Sebastian Falbesoner)
2f79786822 test: refactor: add constant for sequence number `SEQUENCE_FINAL` (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (mempool_accept.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078.
It also includes some other minor changes that came up while working on the replacement:
* [commit 1/4] replace magic number 0xffffffff for a tx's nSequence with a new constant `SEQUENCE_FINAL`
* [commit 2/4] create `CTransaction` instances with the current nVersion=2 by default, in order to use BIP68 for tests
* [commit 3/4] support default `from_node` parameter for creating txs (this is a stripped down version of PR #24025)
ACKs for top commit:
MarcoFalke:
re-ACK aa8a65e4a8📊
Tree-SHA512: 34cd085ea4147ad5bd3f3321c84204064ceb95f382664c7fe29062c1bbc79d9d9465c5e46d35e11c416f2f3cd46030c90a09b518c829c73ae40d060be5e4c9cb
