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
b5c9bb5cb9 util: Restore GetIntArg saturating behavior (James O'Beirne)
Pull request description:
The new locale-independent atoi64 method introduced in #20452 parses large integer values higher than maximum representable value as 0 instead of the maximum value, which breaks backwards compatibility. This commit restores compatibility and adds test coverage for this case in terms of the related GetIntArg and strtoll functions.
Specifically, command line or bitcoin.conf integer values greater than `9223372036854775807` (`2**63-1`) used to be parsed as `9223372036854775807` before #20452. Then #20452 caused them to be parsed as `0`. And after this PR they will be parsed as `9223372036854775807` again.
This change is a stripped-down alternative version of #23841 by jamesob
ACKs for top commit:
jamesob:
Github ACK b5c9bb5cb9
vincenzopalazzo:
ACK b5c9bb5cb9
MarcoFalke:
review ACK b5c9bb5cb9🌘
Tree-SHA512: 4e8abdbabf3cf4713cf5a7c5169539159f359ab4109a4e7e644cc2e9b2b0c3c532fad9f6b772daf015e1c5340ce59280cd9a41f2730afda6099cbf636b7d23ae
The new locale-independent atoi64 method introduced in #20452 parses
large integer values higher than maximum representable value as 0
instead of the maximum value, which breaks backwards compatibility.
This commit restores compatibility and adds test coverage for this case
in terms of the related GetIntArg and strtoll functions.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
If no `from_node` parameter is passed explicitely to the
`create_self_transfer` method, the test node passed in the course
of creating the MiniWallet instance is used. This seems to
be the main use-case in most of the current functional
tests, i.e. in many instances the calls can be shortened.
c9374af102 test: set ban after mocking time (brunoerg)
Pull request description:
Fixes#23988
Set ban after mocking time to avoid intermittent failures related to the assertion of ban_duration and time_remaining.
See: https://cirrus-ci.com/task/6754020390862848?logs=ci#L4652
ACKs for top commit:
mzumsande:
Tested ACK c9374af102
vincenzopalazzo:
ACK c9374af102
Tree-SHA512: fac3ac91a045bb46334d7c568f6a53a3b0a45b306914a54ea13bcc845734eaaad1ff295ff3ab158037fd9d08df77344058331336110b8f7888832b16b0589be5
0754e9c01b test: run feature_pruning.py without wallet compiled (Sebastian Falbesoner)
Pull request description:
Only one small part of the pruning test (sub-test `wallet_test`) is wallet-related, hence we can run all other parts without wallet compiled.
ACKs for top commit:
MarcoFalke:
cr ACK 0754e9c01b
Tree-SHA512: 856856903d21d64953ed0102cc2a96f55975c4b7d8e93e57b82c266024967160df64df2b6068be089efc05e883e8d6d12e7327053420d4c640b9d8cc5bcb1c58
60ae1161a4 qa: replace assert with test framework assertion helpers in fee estimation test (Antoine Poinsot)
e50213967b qa: fee estimation with RBF test cleanups (Antoine Poinsot)
15f5fd62af qa: don't mine non standard txs in fee estimation test (Antoine Poinsot)
eae52dd6ab qa: pass scriptsig directly to txins constructor in fee estimation test (Antoine Poinsot)
1fc03155e5 qa: split coins in a single tx in fee estimation test (Antoine Poinsot)
cc204b8be7 qa: use a single p2sh script in fee estimation test (Antoine Poinsot)
19dd91a9be qa: remove a redundant condition in fee estimation test (Antoine Poinsot)
Pull request description:
Some cleanups that i noticed would be desirable while working on #23074 and #22539, which are intentionally not based on it. Mainly simplifications and a slight speedup.
- Use a single tx to create the `2**9` coins instead of creating `2**8` 2-outputs transactions
- Use a single P2SH script
- Avoid the use of non-standard transactions
- Misc style nits (happy to take more)
ACKs for top commit:
pg156:
I ACK all commits up to 60ae1161a4 (except 1fc03155e5, where I have a question more for my own learning than actually questioning the PR). I built and ran the test successfully. I agree after the changes, the behavior is kept the same and the code is shorter and easier to reason.
glozow:
utACK 60ae1161a4
Tree-SHA512: 57ae2294eb68961ced30f32448c4a530ba1cdee17881594eecb97e1b9ba8927d58c25022b847eb07fb67d676bf436108c416c2f2174864d258fcca5b528b8bbd
fa9f4554ca refactor: Remove pointless and confusing shift in RelayAddress (MarcoFalke)
Pull request description:
The second argument written to the siphash is already quantized to 24 hours, so it seems confusing to quantize the first argument to 32 bits (out of 64 bits).
> The shifting is pointless, we should get rid of it. It seems to be a silly evolution of this 2010 Satoshi code: 5cbf753 (where it made sense because everything was XORed together, and the address used the high bits, while the time used the low ones).
(Copied from https://github.com/bitcoin/bitcoin/pull/18642#issuecomment-613773120)
(The original code was `uint256 hashRand = hashSalt ^ (((int64)addr.ip)<<32) ^ ((GetTime()+addr.ip)/(24*60*60));`)
This also allows to remove a integer sanitizer suppression for the whole file.
ACKs for top commit:
laanwj:
Code review ACK fa9f4554ca
sipa:
utACK fa9f4554ca
promag:
Code review ACK fa9f4554ca.
Tree-SHA512: f5fd107464ccd839d6749aed6914b4935e39ab42906546b3f3810a7339fc4633fef931a1783a287572af5ec64525626fa91d147d8ff52eb076740465bf5cf839
ac617cc141 wallettool: Check that the dumpfile checksum is the correct size (Andrew Chow)
Pull request description:
After parsing the checksum, make sure that it is the size that we expect it to be.
This issue was reported by Pedro Baptista.
ACKs for top commit:
laanwj:
Code review ACK ac617cc141
Tree-SHA512: 8135b3fb1f4f6b6c91cfbac7d1d3421f1f6c664a742c92940f68eae857f92ce49d042cc3aa5c2df6ef182825271483d65efc7543ec7a8ff047fd7c08666c8899
8bd34dc774 test: check that bitcoin-tx detects missing input amount for segwit transactions (Sebastian Falbesoner)
c337b27d7c Require that input amount is provided for bitcoin-tx witness transactions (Ben Woosley)
Pull request description:
This PR picks up the obviously abandoned PR #13608 (last activity was three and a half years ago) by rebasing it on master and adding missing tests. Original PR description: "_Applies fix from #12458 / #13547 to bitcoin-tx._"
The private key is the compressed version of the one used in most other util tests (5HpHagT65TZzG1PH3CSu63k8DbpvD8s5ip4nEB3kEsreAnchuDf, corresponds to the scalar value k=1 in big endian), since segwit signing refuses uncompressed keys.
The error message from the picked up PR is changed to not include the amount, as showing any value would be just confusing.
ACKs for top commit:
josibake:
ACK 8bd34dc774
Tree-SHA512: 334b418f89527363ad7e3326b4126e86a05fd64876c49a8280de38e64cfac52cb62c4b24b83603dd68b6bcebbe57c64161832edffb1cac7e9c68426f6b6eae1f
975097f424 Let test_runner.py start multiple jobs per timeslot (Pieter Wuille)
Pull request description:
test_runner.py currently only checks every 0.5s whether any job has finished, and if so, starts at most one new job. At higher parallellism it becomes increasingly likely that multiple jobs have finished at the same time. Fix this by always noticing *all* finished jobs every timeslot, and starting as many new ones.
ACKs for top commit:
laanwj:
Code review and lightly tested ACK 975097f424
prayank23:
ACK 975097f424
Tree-SHA512: b70c51f05efcde9bc25475c192b86e86b4c399495b42dee20576af3e6b99e8298be8b9e82146abdabbaedb24a13ee158a7c8947518b16fc4f33a3b434935b550
7746606cfa test: use MiniWallet for mining_basic.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (mining_basic.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078.
ACKs for top commit:
brunoerg:
crACK 7746606cfa
Tree-SHA512: 4455b8b764413b0fc3ef388e3c5d5758f9e6b6d3193ac660269a9ba1c988022e6b7bc148549c2167942ea472c5aaddd2b6b3b9d99790d0654b089af975b86e11
8904f17ea7 test: enable txindex in feature_init (James O'Beirne)
93db6d8422 test: feature_init: retain debug.log and improve detection (James O'Beirne)
24fcf6e435 test: feature_init: tweak all .ldb files of a certain type (James O'Beirne)
a8ffbc01db test: add TestNode.wait_for_debug_log (James O'Beirne)
a2fb62b632 test: introduce TestNode.debug_log_bytes (James O'Beirne)
Pull request description:
Hopefully fixes#23646.
This makes a few changes to feature_init (along the lines of the discussion here: https://github.com/bitcoin/bitcoin/pull/23289#discussion_r766615073). The changes are detailed in the specific commits, but a summary might be:
- more robust log-line detection (at the expense of maybe being slower)
- retain all debug.log content
- perturb .ldb files in a more complete way
Top commit has no ACKs.
Tree-SHA512: 9c8900998fdb96d3cc92bd30c4eeda2a47e9787e6432546d458c17f24eb9e2b6b3eb573db225172888ca71d7a62260f497f83827597b982b9e35c5452cb6e514
126853214a test: add functional test for -startupnotify (Bruno Garcia)
Pull request description:
This PR adds a functional test for -startupnotify. It basically starts the node passing a command on -startupnotify to create a file on tmp and then, we check if the file has been successfully created.
ACKs for top commit:
theStack:
Tested ACK 126853214a
kristapsk:
re-ACK 126853214a
Tree-SHA512: 5bf3e46124ee5c9d609c9993e6465d5a71a8d2275dcf07c8ce0549f013f7f8863d483b46b7164152f566468a689371ccb87f01cf118c3c9cac5b2be673b61a5c
fadd73037e refactor: Remove implicit-integer-sign-change suppressions in validation.cpp (MarcoFalke)
Pull request description:
A file-wide suppression is problematic because it will wave through future violations, potentially bugs.
Fix that by using per-statement casts.
ACKs for top commit:
shaavan:
ACK fadd73037e
theStack:
Code-review ACK fadd73037e
Tree-SHA512: a8a05613be35382b92d7970f958a4e8f4332432056eaa9d72f6719495134b93aaaeea692899d9035654d0e0cf56bcd759671eeeacfd0535582c0ea048ab58a56
4523d28b6b [test] compare filter and header with the result of the getblockfilter RPC (Niklas Gögge)
3a2464f216 [rest] drop superfluous rpc serializations flags for block filters (Niklas Gögge)
064abd14a5 [rest] add a more verbose error message for invalid header counts (Niklas Gögge)
83b8f3a896 [refactor] various style fix-ups (Niklas Gögge)
Pull request description:
This PR addresses unresolved review comments from [#17631](https://github.com/bitcoin/bitcoin/pull/17631).
This includes:
* various style fix-ups
* returning a more verbose error message for invalid header counts
* removing superfluous rpc serializations flags for block filters
* improving the test to include comparing the block filters returned from the rest with the ones returned from the `getblockfilter` RPC.
ACKs for top commit:
jnewbery:
ACK 4523d28b6b
brunoerg:
tACK 4523d28b6b
Tree-SHA512: 634e6b2ae3e1d5f31675a50cfe11a5e74bf5a51b9e7b512d9e18879bf2ed424fc0ac6ec633023f192e3ad12cf0c73b0b51de79dd7ec00844dba3e1493d823b8c
1362d6173f scripted-diff: Insert missed copyright headers (Hennadii Stepanov)
f47dda2c58 scripted-diff: Bump copyright headers (Hennadii Stepanov)
c29105efdc script: Fix copyright_header.py (Hennadii Stepanov)
Pull request description:
This PR is an alternative to #23903.
It bumps the existing copyright headers as we did every year, and adds the missed copyright headers.
A small fix has been applied to the `copyright_header.py` in order to prevent such weird bumping as `2021` --> `2021-2017`.
ACKs for top commit:
MarcoFalke:
ACK 1362d6173f
Tree-SHA512: 204d970fe8c51546b26b8f03fe4297db8a9bef5101df851540b7b9eddbd3a09677ee81fdd882c60937d732407f42c9883165bd978272200cff8f90190f075905
da349f131a test: check ban_duration and time_remaining after setting ban (brunoerg)
Pull request description:
This PR adds functional test coverage for `ban_duration` and `time_remaining` introduced in #21602
ACKs for top commit:
shaavan:
ACK da349f131a
theStack:
Tested ACK da349f131a
Tree-SHA512: 51e63f3a36adb1c81e4d49426486af2cd9c8c4319f94e06a47fa7da8100a8b53c029d28d4a4771bdbf4e0a2bfb4ddd3740b9974bd08d8ff06f2a0fc2b6d8a6b5
This test sporadically fails due to the Python test missing log lines
for reasons that are poorly understood. The problem is made worse by the
fact that this test does not retain the log files from iteration to
iteration.
Change the test to do logline detection in a more robust manner (by
using `re.search` on the whole log content) in a way that is comparable
to the existing `assert_debug_log` utility, and retain all debug.log
content from case to case.
This part of the test sporadically fails on CI infrastructure. Instead
of perturbing a single .ldb file of each type, move all .ldb files of a
given type to ensure a bad startup.
This serves as a replacement for the getnewaddress RPC if no wallet is
available. In addition to the address, it also returns the corresponding
public key and output script (scriptPubKey).
fafe4dea16 test: Fix pep8 of touched file (MarcoFalke)
fa0ac9d7e3 test: Fix rpc_scantxoutset intermittent issue (MarcoFalke)
Pull request description:
I fail to see how this could have ever worked, since there is nothing that prevents the wallet from spending the coins in the utxo set.
Fixes https://github.com/bitcoin/bitcoin/issues/23847
Longer term it would be nice to remove the wallet and use MiniWallet here.
ACKs for top commit:
brunoerg:
tACK fafe4dea16
theStack:
Code-review ACK fafe4dea16
Tree-SHA512: d92b7be9a81eb62f496488dd15b8e564e9b7a64b55634af2714b53f985e6a35fdae788323833ff59c40ae7c6a0cf54fe069db8eb3be03686f7c100379f54a292
fada6c65d2 wallet: Strictly match tx change type to improve privacy (MarcoFalke)
Pull request description:
Currently the change type will only match a destination by accident, making it easier to determine the change.
Fix that by strictly matching one of the destinations.
ACKs for top commit:
S3RK:
Concept & Approach ACK fada6c6. Also did light code review .
achow101:
ACK fada6c65d2
prayank23:
tACK fada6c65d2
w0xlt:
tACK fada6c6
Tree-SHA512: 2b072c3c32debac7b0bef07a6df9a8f1a631e0f7d556b859973f18894ca490225582dc13e4588b29fa205ffbcd30fb632d5313b304d10ad17a26adc3f7684471
140a49ce5e test: check that pruneblockchain RPC fails for future block or timestamp (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `pruneblockchain` RPC for the case that a future block or timestamp is passed:
8c0bd871fc/src/rpc/blockchain.cpp (L1101)8c0bd871fc/src/rpc/blockchain.cpp (L1111)
Note that the test method `manual_test` gets called twice, once each with `use_timestamp` set to True/False, respectively. Depending on that, the helper function `height` either converts the passed block height to the timestamp of that block, or just returns it without modification.
The other tests for failures in this RPC are also changed to be more detailled ("Cannot prune blocks because node is not in prune mode", "Negative block height"), as I don't think there is any value in just checking a sub-string. If there is ever an error with the same sub-string is introduced, it's not clear which error is exactly checked with the test, so it makes sense to be as specific as possible.
ACKs for top commit:
brunoerg:
tACK 140a49ce5e
Tree-SHA512: bee3cee9f35c2a63a1839d7ec1f83e354d9d3c0c2ca32d300dca2de8b755d555f769ba2b80ac37d31df6ee7e2b8eaefb8134c4727a7144e47c0f5e34f2cc5822
0a1b6fa5a1 test: fix intermittent timeouts in p2p_timeouts.py (Martin Zumsande)
Pull request description:
Fixes #23800 by making sure that all peers are connected (i.e. `m_connected` is set) before the mocktime is bumped.
We can't wait for verack here, but we can wait for a debug log entry ("Added connection peer=2") instead.
In the failed CI runs (e.g. https://cirrus-ci.com/task/5600553806856192?logs=ci#L7469) different peers were added at different mocktimes.
ACKs for top commit:
naumenkogs:
ACK 0a1b6fa5a1
theStack:
Concept and approach ACK 0a1b6fa5a1
Tree-SHA512: 1a3c8a9a79339d4adc6ecb1731eb0d0eadb2e5024ad3c6779b4696691f85d6c3304ef8689746d0332150a4cf04489ca4b2ff3eeb0bb76feec28c1e4bb9dbca19
2b64fa3251 Update REST docs with new accessors (Matt Corallo)
ef7c8228fd Expose block filters over REST. (Matt Corallo)
Pull request description:
This adds a new rest endpoint:
/rest/blockfilter/filtertype/requesttype/blockhash (eg
/rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
which exposes either the filter "header" or the filter data itself.
Most of the code is cribbed from the equivalent RPC.
You can test it at 000000005b.hex
ACKs for top commit:
dergoegge:
ACK 2b64fa3251 - Adding blockfilters to the REST interface is analogous to serving other public data such as transactions or blocks.
Tree-SHA512: d487bc694266375c94d6fcf2e9d788a8a42a3b94e8d3290e46335a64cbcde55084ce5ea6119b79a4065888d94d7c3ae25a59a901fa46e3711f0eb296add12696
261dddb924 test: Combine addr generation helper functions (Martin Zumsande)
aeeccd9aa6 test: Fix intermittent issue in p2p_addr_relay.py (Martin Zumsande)
Pull request description:
Fixes#22449 by increasing the mocktime jump (just as in 6168eb06b2), which prevents failures due to rare Poisson timer events, or at least makes them a lot more unlikely.
The second commit combines the addr generation helper functions `setup_addr_msg` and `setup_rand_addr_msg`. It also changes the way `addr.time` is filled to random, because before, if too many addresses (>600) were created in a batch, they would stop being relayed because their timestamp would be too far in the future.
ACKs for top commit:
josibake:
reACK 261dddb924
jnewbery:
utACK 261dddb924
Tree-SHA512: d0eca887de4bc85092730284cf612193d2c12b0a3d624a2bfa5fef4a5890d3b6375c564333c5927425958e4b6ec86b8854b18b2233c7b6f1691d9ddc397948a9
This combines the addr generation helper functions setup_addr_msg
and setup_rand_addr_msg.
It also changes the way addr.time is filled to random, because before,
if too many addresses (>600) were created in a batch, they would stop
being relayed because their timestamp would be too far in the future.
62fa61fa4a refactor: remove the wallet folder if the restore fails (w0xlt)
abbb7eccef refactor: Move restorewallet() RPC logic to the wallet section (w0xlt)
4807f73f48 refactor: Implement restorewallet() logic in the wallet section (w0xlt)
Pull request description:
Currently `restorewallet()` logic is written in the RPC layer and it can´t be reused by GUI. So it moves this to the wallet section and then, GUI can access it.
This is necessary to implement the "Restore Wallet" menu item in the GUI (which is already implemented in https://github.com/bitcoin-core/gui/pull/471 ).
This commit also simplifies error handling and adds a new behavior: if the restore fails, the invalid wallet folder is removed.
ACKs for top commit:
achow101:
ACK 62fa61fa4a
shaavan:
crACK 62fa61fa4a
Tree-SHA512: 7ccfbad5943f38616ba0c2dd443c97a4b5bc1f6612dbf5a9e7a0263100aba36671fae929a2e7688442667be394645f44484af137a4802f204a33c4689eb27c39
618f4d2890 test: re-organized array according to order of logs and included 2 more interruption events (seaona)
71115a5e23 test: include two more interruptions points (seaona)
Pull request description:
This PR aims to introduce 2 more interruption points in the process of initialization, in order to make the` feature_init `testcase more complete. These are the following:
-` Checking all blk files are present`
-` init message: Starting network threads`
It is a small improvement for increasing the coverage of potential interruptions, and making sure that the node can restart successfully after these interruptions.
ACKs for top commit:
jamesob:
ACK 618f4d2890, pending CI
jarolrod:
ACK 618f4d2890
Tree-SHA512: 9d709734e298e955709094bb97478ca7f18859874f1ba026f7c9014d87205aea63f6cf2093ebee600eaf82d3245adb11e77fae24a1ae48b69efefd57f3def921
The -sandbox argument is not present in the v22.0 release. Changing the minimum version to 229900 ensures it's used when testing the master branch.
If the argument is backported, the minimum version can be adjusted to e.g. 220100.
This cleanup may slightly impact test coverage.
Reduce code repition by looping over the list of nodes.
Reduce brittleness by refering to nodes by name rather than index.
Add helper method nodes_wallet_dir.
It is not possible to run the compatibility tests on i686 because the
releases v20+ are missing for that arch. It would be possible to
self-compile those releases, but then one can also self-compile
0.15-0.19.
fad943821e scripted-diff: Rename touched member variables (MarcoFalke)
fa663a4c0d Use mockable time for peer connection time (MarcoFalke)
fad7ead146 refactor: Use type-safe std::chrono in net (MarcoFalke)
Pull request description:
Benefits:
* Type-safe
* Mockable
* Allows to revert a temporary test workaround
ACKs for top commit:
naumenkogs:
ACK fad943821e
shaavan:
ACK fad943821e
Tree-SHA512: af9bdfc695ab727b100c6810a7289d29b02b0ea9fa4fee9cc1f3eeefb52c8c465ea2734bae0c1c63b3b0d6264ba2c493268bc970ef6916570eb166de77829d82
fa5865a9e3 Reduce size of strencodings decode tables (MarcoFalke)
fad6761cf7 Fix implicit integer sign changes in strencodings (MarcoFalke)
Pull request description:
This removes casts where they are nonsensical (ToUpper/ToLower) and adds them where needed. Then, remove the suppressions.
Also, reduce static memory usage.
ACKs for top commit:
shaavan:
crACK fa5865a9e3
Tree-SHA512: 29850fb616789befad17f71731f322b2238e94ec431cda293629de540f7ab02060a9fc5b7666168ca2592048df5a39a2975177f47771d4d8211d90321052549d
eaf6be0114 [net processing] Do not request transaction relay from feeler connections (John Newbery)
0220b834b1 [test] Add testing for outbound feeler connections (John Newbery)
Pull request description:
Feelers are short-lived connections used to test the viability of peers. The bitcoind node will periodically open feeler connections to addresses in its addrman, wait for a `version` message from the peer, and then close the connection.
Currently, we set `fRelay` to `1` in the `version` message for feeler connections, indicating that we want the peer to relay transactions to us. However, we close the connection immediately on receipt of the `version` message, and so never process any incoming transaction announcements. This PR changes that behaviour to instead set `fRelay` to `0` indicating that we do not wish to receive transaction announcements from the peer.
This PR also extends the `addconnection` RPC to allow creating outbound feeler connections from the node to the test framework, and a test to verify that the node sets `fRelay` to `0` in the `version` message to feeler connections.
ACKs for top commit:
naumenkogs:
ACK eaf6be0114
MarcoFalke:
review ACK eaf6be0114🏃
Tree-SHA512: 1c56837dbd0a396fe404a5e39f7459864d15f666664d6b35ad109628b13158e077e417e586bf48946a23bd5cbe63716cb4bf22cdf8781b74dfce6047b87b465a
fa24a3df87 rpc: Quote user supplied strings in error messages (MarcoFalke)
Pull request description:
I can't see a downside doing this and this fixes a fuzzing crash
Background:
This is a follow-up to commit 926fc2a0d4, which introduced the "starts_with-hack". Maybe an alternative to the hack would be to assign a unique error code to internal bugs? However, I think this can be done in an separate pull request and the changes here make sense even on their own.
ACKs for top commit:
fanquake:
ACK fa24a3df87 - to fix the fuzzers.
Tree-SHA512: d998626406a64396a037a6d1fce22fce3dadb7567c2f9638e450ebe8fb8ae77d134e15dd02555326732208f698d77b0028bc62be9ceee9c43282b61fe95fccbd
41b9f7d062 test: Use byte unit 'M' for -maxuploadtarget functional test (Douglas Chimento)
Pull request description:
ACKs for top commit:
shaavan:
ACK 41b9f7d062
stratospher:
ACK 41b9f7d.
Tree-SHA512: 25b46347c671e8d6fd8878e7fee40e773bb03641e53e41e8a79a286fe4a0cf71c0c0986d6d7418fcb656c07f7216cc50a7ee4366f9213c32b01ae74326031f80
9600ea0145 test: Add edge case of pruning up to index height (Martin Zumsande)
698c524698 index: Fix backwards search for bestblock (Martin Zumsande)
Pull request description:
This PR attempts to fix an intermittent Init issue encountered during the stress testing of #23289, which relates to the pruning-compatible filter reconstruction logic introduced in #15946.
The problem would occur when the node starts with `-txindex=1` but `ThreadSync` is interrupted after it sets `m_best_block_index` to Genesis, and before it gets do any further work.
In that case, during the next restart of the node, an Init error would be thrown because `BaseIndex::Init()` tries to backtrack from the tip to the last block which has been successfully indexed (here: Genesis), but the backtracking logic didn't work properly in this case:
The loop
`while (block_to_test && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))` checks if a predecessor exists **before** performing the check `block_to_test == block` and then possbily setting `prune_violation = false`
If `block_to_test` and `block` are the Genesis block this check will not be reached because `block->pprev` does not exist.
To reproduce this bug on regtest:
1) start a node with a fresh datadir using `-txindex=1` (or any other index)
2) stop and restart without any index
3) mine a block
3) stop and restart again with the index enabled
->InitError `Error: txindex best block of the index goes beyond pruned data. (...)`
Fix this by requiring that we have the data for the block of the current iteration `block` (instead of requiring it for the predecessor `block->pprev`)
That way, the check for `block_to_test == block` is also reached when `block_to_test` is the Genesis block.
No longer requiring the data of `block->pprev` also means that we can now prune up to `m_best_block_index` height without requiring a reindex (one block more than before). I added this edge case to `feature_blockfilterindex_prune.py`, the new version should fail on master.
ACKs for top commit:
ryanofsky:
Partial code review ACK 9600ea0145 for the code change, not the test changes. (Test changes are indirect and little over my head.) It seems obvious that previous code `prune_violation = true, while (block->pprev)` would incorrectly detect a prune violation at the genesis block, and the fix here make sense and looks correct.
Tree-SHA512: c717f372cee8fd49718b1b18bfe237aa6ba3ff4468588c10e1272d7a2ef3981d10af4e57de51dec295e2ca72d441bc6c2812f7990011a94d7f818775e3ff1a38
d5cab1a96d Add createmultisig and addmultisigaddress warnings release note (Samuel Dobson)
e46fc935aa Add warnings field to addmultisigaddress to warn about uncompressed keys (Samuel Dobson)
d1a9742623 Add warnings field to createmultisig to warn about uncompressed keys (Samuel Dobson)
Pull request description:
Fixes#21368
Currently, if there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. Rather than keeping this silent behaviour which may be confusing to users, we explicitly add a `warnings` field which will warn the user why their address format is different.
ACKs for top commit:
achow101:
ACK d5cab1a96d
Tree-SHA512: c2ac7f7689251bd4fcd8c26506f053921fbaf34c7a26a74e82ebc7f82cc0bd25407fd7954bf98365dcafa51fa45dcdbee6214320580ca69509690c3555e71cc0
c0405ee27f rpc: Document that DEFAULT is for Taproot, ALL for everything else (Andrew Chow)
d3992669df psbt: Actually use SIGHASH_DEFAULT (Andrew Chow)
eb9a1a2c59 psbt: Make sighash_type std::optional<int> (Andrew Chow)
Pull request description:
Make the behavior align with the help text by actually using SIGHASH_DEFAULT as the default sighash for signing PSBTs.
ACKs for top commit:
Sjors:
re-utACK c0405ee27f
Tree-SHA512: 5199fb41de416b2f10ac451f824e7c94b428ba11fdb9e50f0027c692e959ce5813a340c34a4e52d7aa128e12008303d80939a693eff36a869720e45442119828
fadc0c80ae p2p: Make timeout mockable and type safe, speed up test (MarcoFalke)
fa6d5a238d scripted-diff: Rename m_last_send and m_last_recv (MarcoFalke)
Pull request description:
Use type-safe time for better code readability/maintainability and mockable time for better testability. This speeds up the p2p_timeout test.
This is also a bugfix for intermittent test issues like: https://cirrus-ci.com/task/4769904156999680?command=ci#L2836Fixes#20654
ACKs for top commit:
laanwj:
Code review ACK fadc0c80ae
naumenkogs:
ACK fadc0c80ae
Tree-SHA512: 28c6544c97f188c8a0fbc80411c74ab74ffd055885322c325aa3d1c404b29c3fd70a737e86083eecae58ef394db1cb56bc122d06cff63742aa89a8e868730c64
5493e92501 Check descriptors returned by external signers (sstone)
Pull request description:
Check that descriptors returned by external signers have been parsed properly when creating a new wallet.
See https://github.com/bitcoin/bitcoin/issues/23627 for context.
The problem is that parsing an invalid descriptor will return `null` which is not checked for in `CWallet::SetupDescriptorScriptPubKeyMans()`.
I'm not completely sure what the best fix is since there several strategies for dealing with errors in the current codebase but the proposed fix is very simple and consistent with other validation checks in `CWallet::SetupDescriptorScriptPubKeyMans()`.
ACKs for top commit:
jamesob:
Code review ACK 5493e92501
achow101:
ACK 5493e92501
Tree-SHA512: 63259f4aa519405a86c554b6813efdb741314bdaa18bf005b70ea8bb92a27abc6e2b65f7c584641dc257fc78a6499f42b51b5310c243e611c4663430dccf3d04
This simplifies the code, and slightly speeds up the test.
Running `./test/functional/test_runner.py -j15 $(printf 'feature_fee_estimation %.0s' {1..15})`
on master 3 times gives:
- Before:
ALL | ✓ Passed | 788 s (accumulated)
ALL | ✓ Passed | 818 s (accumulated)
ALL | ✓ Passed | 873 s (accumulated)
- After:
ALL | ✓ Passed | 763 s (accumulated)
ALL | ✓ Passed | 798 s (accumulated)
ALL | ✓ Passed | 731 s (accumulated)
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
84bc35d7a5 test: feature_rbf.py: check specified wallet type availability (Sebastian Falbesoner)
Pull request description:
The test currently leads to a failure if in general wallet support is compiled, but the library for the specified type (BDB/SQLite) is not, i.e. if started with the `--legacy-wallet` parameter, but bitcoind is compiled without BDB support, see e.g. https://github.com/bitcoin/bitcoin/pull/23682#issuecomment-989044207
Fix this by checking if the specified wallet type (BDB for legacy wallet, SQLite for descriptor wallet) is available.
Also move the helper `is_specified_wallet_compiled()` to the
test framework's class BitcoinTestFramework first, so it can be reused.
Should further pave the way for #23682. On my local instance without BDB compiled, all targets in test_runner pass now.
ACKs for top commit:
mzumsande:
ACK 84bc35d7a5, changes loook good for me and I confirmed that this fixes the error encountered on master when running the test `--without-bdb`.
Tree-SHA512: 1575c03c793c8e0ac195d0914eff75d02604301c8fb77d0fdb7c0b245561569c0c7db387ef4de499044b68ab6e14b4b78b955f6e74c84197bcaed95f439c9824
5b559dc7ec Swap out hashlib.ripemd160 for own implementation (Pieter Wuille)
ad3e9e1f21 Add pure Python RIPEMD-160 (Pieter Wuille)
Pull request description:
Closes#23710.
ACKs for top commit:
jamesob:
ACK 5b559dc7ec, pending CI
Tree-SHA512: dcd4ea2027eac572f7ab0da434b081b9a5d6b78675e559258a446b4d254b29d93c4d2cc12da4a28303543d6d99f5f2246fde4052e84af81d18e04399b137b39e
The test currently leads to a failure if in general wallet
support is compiled, but the library for the specified type
(BDB/SQLite) is not, i.e. if started with the
`--legacy-wallet` parameter, but bitcoind is compiled
without BDB support.
Fix this by checking if the specified wallet type (BDB for
legacy wallet, SQLite for descriptor wallet) is available.
Also move the helper `is_specified_wallet_compiled()` to the
test framework's class BitcoinTestFramework first, so it can
be reused.
Currently the test performs the wallet-relevant parts if
_any_ wallet type support is compiled in, independently of
whether the test is run with legacy or descriptor wallet
specified. This leads to a failure if the test is started
with the `--legacy-wallet` parameter, but bitcoind is
compiled without BDB support.
Fix this by checking if the specified wallet type (BDB for
legacy wallet, SQLite for descriptor wallet) is available.
dce8c4c381 rpc: getblockfrompeer (Sjors Provoost)
b884ababc2 rpc: move Ensure* helpers to server_util.h (Sjors Provoost)
Pull request description:
This adds an RPC method to fetch a block directly from a peer. This can used to fetch stale blocks with lower proof of work that are normally ignored by the node (`headers-only` in `getchaintips`).
Usage:
```
bitcoin-cli getblockfrompeer HASH peer_n
```
Closes#20155
Limitations:
* you have to specify which peer to fetch the block from
* the node must already have the header
ACKs for top commit:
jnewbery:
ACK dce8c4c381
fjahr:
re-ACK dce8c4c381
Tree-SHA512: 843ba2b7a308f640770d624d0aa3265fdc5c6ea48e8db32269b96a082b7420f7953d1d8d1ef2e6529392c7172dded9d15639fbc9c24e7bfa5cfb79e13a5498c8
1dcba996d3 Coinbase receivedby rpcs release notes (Andrew Toth)
b5696750a9 Test including coinbase transactions in receivedby wallet rpcs (Andrew Toth)
bce20c34d6 Include coinbase transactions in receivedby wallet rpcs (Andrew Toth)
Pull request description:
The current `*receivedby*` RPCs filter out coinbase transactions. This doesn't seem correct since an output to your address in a coinbase transaction *is* receiving those coins.
This PR corrects this behaviour. Also, a new option `include_immature_coinbase` is added (default=`false`) that includes immature coinbase transactions when set to true.
However, since this is potentially a breaking change this PR introduces a hidden configuration option `-deprecatedrpc=exclude_coinbase`. This can be set to revert to previous behaviour. If no reports of broken workflow are received, then this option can be removed in a future release.
Fixes https://github.com/bitcoin/bitcoin/issues/14654.
ACKs for top commit:
jnewbery:
reACK 1dcba996d3
Tree-SHA512: bfc43b81279fea5b6770a4620b196f6bc7c818d221b228623e9f535ec75a2406bc440e3df911608a3680f11ab64c5a4103917162114f5ff7c4ca8ab07bb9d3df
