d8fe5f0326 test: improve interface_ipc.py waitNext tests (Ryan Ofsky)
a5e61b1917 test: interface_ipc.py minor fixes and cleanup (Ryan Ofsky)
ded11fb04d test: fix interface_ipc.py template destruction (Ryan Ofsky)
Pull request description:
This PR cleans up the `interface_ipc.py` test, fixing broken checks, fixing missing await calls, removing to_dict calls, renaming variables, reducing `.result` accesses, and giving template objects explicit lifetimes. More details are in the commit messages.
The first commit changes a lot of indentation so is easiest to review ignoring whitespace.
ACKs for top commit:
Sjors:
ACK d8fe5f0326
sedited:
ACK d8fe5f0326
Tree-SHA512: f0de309a15cb23f109cf6909e51ddd132a60bd4d4cb25b20bdc74545516670f1cdb0c9cc98c397c2f24e67e2380c2dac9d00435009618a3c00b6b85cca5c3e2e
09dfa4d3f8 test: fix race condition in p2p_v2_misbehaving.py peerid assertion (stratospher)
Pull request description:
Remove the hard-coded peer id from the debug message in `p2p_v2_misbehaving.py`.
asyncio's non-deterministic task scheduling might cause [peer2](938d7aacab/test/functional/p2p_v2_misbehaving.py (L181))'s connection to happen before [peer1](938d7aacab/test/functional/p2p_v2_misbehaving.py (L179))'s. since we test that peer2 [remains connected](938d7aacab/test/functional/p2p_v2_misbehaving.py (L182)), any disconnection must originate from peer1, making the specific peer id not necessary for test correctness. so we can remove the hard coded peer id from the expected debug log message.
Fixes#34035.
ACKs for top commit:
maflcko:
lgtm ACK 09dfa4d3f8
mzumsande:
Code Review ACK 09dfa4d3f8
Tree-SHA512: 542b08ddae09db7454e8c08b1d26aade50a53c2505683df99556cf071a6a38195b64f8700f6db3f4e1b318497fc4b5232246ad4e9d6f3af45fad83e333fa91fb
fa8a5d215c log: Remove brittle and confusing LogPrintLevel (MarcoFalke)
fac24bbec8 test: Clarify logging_SeverityLevels test (MarcoFalke)
f273167661 ipc: separate log statements per level (stickies-v)
94c51ae540 libevent: separate log statements per level (stickies-v)
Pull request description:
`LogPrintLevel` has many issues:
* It encourages to log several levels in one source location. This is problematic, because all levels (even warnings and errors) will be rate limited equally for the same location.
* Its warning and error logs are specially formatted compared to all other warning and error logs in the codebase, making them harder to spot (both in the debug log and in the code).
* It is verbose to type and read.
* It is confusing, because the majority of code uses the `Log$LEVEL(...)` macros. Having less ways to achieve the same makes the code more consistent and easier to review.
Fix all issues by removing it
ACKs for top commit:
stickies-v:
re-ACK fa8a5d215c
ajtowns:
ACK fa8a5d215c
pablomartin4btc:
re-ACK fa8a5d215c
Tree-SHA512: 9fbb04962d9c26e566338694a7725b3c0e88ef733322d890bcc6aeddb45266c754e7c885c69bbfebd1588cc09912c6784cfc00e69882f1271a8c87d201490478
due to asyncio's non-deterministic task scheduling, peer2's
connection might happen before peer1's, causing peer2 to get
assigned peer_id=1 on bitcoind side and peer1 to get assigned
peer_id=2 on bitcoind side.
since we test that peer2 remains connected, any disconnection
must originate from peer1, making the specific peer id unnecessary
for test correctness. so we can remove the specific peer_id from
the expected debug log.
fa75480c84 test: Detect truncated download in get_previous_releases.py (MarcoFalke)
Pull request description:
Without this, and end-of-stream is not detected and will just lead to an immediate exit, instead of a re-try.
E.g. https://github.com/bitcoin/bitcoin/actions/runs/20089133013/job/57633839315?pr=34038#step:12:201:
```
...
Downloading: [##--------------------------------------] 5.4%
Downloading: [##--------------------------------------] 5.4%
Downloading: [##--------------------------------------] 5.5%
Downloading: [##--------------------------------------] 5.6%
Checksum dd02eab18f9154604e38135ef3f98fd310ba3c748074aeb83a71118cd2cd1367 did not match
Error: Process completed with exit code 1.
```
Also, remove the `0` fallback value, because if the fallback was ever hit, the program would fail anyway with `division by zero` error.
ACKs for top commit:
Sjors:
utACK fa75480c84
rkrux:
Looks fine, ACK fa75480c84
l0rinc:
code review ACK fa75480c84
Tree-SHA512: 230eaf155701ed833636b401118f11ff5c6521c61bf4f3a01fcf390a71a508ba6a570eea855ef659134e118b74f75e3d5772ec8a261db23ebfe4ac7ec87cab5a
e7ac5a133c doc: add release note for 34031 (fanquake)
c4c70a256e netbase: Remove "tor" as a network specification (Carl Dong)
Pull request description:
"tor" as a network specification was deprecated in 60dc8e4208 in favor of "onion"
and this commit removes it and updates the relevant test.
Previously #16029. This has been warning as being deprecated since `v0.17.0`.
This PR only removes the already deprecated usage of tor as a network specification, the use of tor throughout the codebase, is not deprecated.
ACKs for top commit:
davidgumberg:
crACK e7ac5a133c
laanwj:
Code review ACK e7ac5a133c
janb84:
ACK e7ac5a133c
stickies-v:
ACK e7ac5a133c
Tree-SHA512: f211dec151c21728b4cd2b1716ee68907871beaa85d8c89e2bc17576e701d03c03e5455593de94970d787aa3264fab60d8c6debeeff908e00d8feb48804692e9
a1f7623020 qa: Only complain about expected messages that were not found (Hodlinator)
1e54125e2e refactor(qa): Avoid unnecessary string operations (Hodlinator)
a9021101dc qa: Replace always-escaped regexps with "X in Y" (Hodlinator)
5c16e4631c doc: Remove no longer correct comment (Hodlinator)
Pull request description:
* Remove incorrect docstring in `busy_wait_for_debug_log()`.
* Replace nerfed regex searches with `X in Y` expressions.
* Only compute the log string to be printed on failure *when we actually fail* instead of every 0.05s.
* As we find each needle (expected message) in the haystack (log output), stop searching for it. **If we fail and time out, we will only complain about the needles (expected messages) we didn't find. On master we also include found needles, which is less helpful.**
Found while developing a new test case in https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351892330
ACKs for top commit:
l0rinc:
Code review ACK a1f7623020
maflcko:
review ACK a1f7623020💨
Tree-SHA512: 191ea7647b0ea8b4220e37c62d176861c2fd0e3737aee3641b262915d9118f48953cf1204767c93a93a8fc78a44c2c29206fb390b44c59d99fc2aa7d12bf4889
4b47113698 validation: Reword CheckForkWarningConditions and call it also during IBD and at startup (Martin Zumsande)
2f51951d03 p2p: Add warning message when receiving headers for blocks cached as invalid (Martin Zumsande)
Pull request description:
In case of corruption that leads to a block being marked as invalid that is seen as valid by the rest of the network, the user currently doesn't receive good error messages, but will often be stuck in an endless headers-sync loop with no explanation (#26391).
This PR improves warnings in two ways:
- When we receive a header that is already saved in our disk, but invalid, add a warning. This will happen repeatedly during the headerssync loop (see https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-1291765534 on how to trigger it artificially).
- Removes the IBD check from `CheckForkWarningConditions` and adds a call to the function during init (`LoadChainTip()`). The existing check was added in 55ed3f1475 a long time ago when we had more sophisticated fork detection that could lead to false positives during IBD, but that logic was removed in fa62304c97 so that I don't see a reason to suppress the warning anymore.
Fixes#26391 (We'll still do the endless looping, trying to find a peer with a headers that we can use, but will now repeatedly log warnings while doing so).
ACKs for top commit:
glozow:
ACK `git range-diff 6d2c8ea9dbd77c71051935b5ab59224487509559...4b4711369880369729893ba7baef11ba2a36cf4b`
theStack:
re-ACK 4b47113698
sedited:
ACK 4b47113698
Tree-SHA512: 78bc53606374636d616ee10fdce0324adcc9bcee2806a7e13c9471e4c02ef00925ce6daef303bc153b7fcf5a8528fb4263c875b71d2e965f7c4332304bc4d922
a7c96f874d tests: Add witness commitment if we have a witness transaction in FullBlockTest.update_block() (Chris Stewart)
Pull request description:
This is useful for test cases where we want to test logic invalid blocks that contain witness transactions. If we don't add the witness commitment as per [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#user-content-Commitment_structure), blocks will be rejected with the error [`Block mutated`](fb0ada982a/src/validation.cpp (L4180)).
This change was needed in https://github.com/ajtowns/bitcoin/pull/13 which is a soft fork proposal to disallow 64 byte transactions. We want to test that 64 byte transactions serialized without the witness are invalid. If we do not have this change, we cannot directly test the logic that rejects 64 byte transactions.
I decided to PR this upstream as many soft fork proposals may not see the light of day, but this functionality seems strictly additive to the test framework.
ACKs for top commit:
theStack:
ACK a7c96f874d
sedited:
ACK a7c96f874d
glozow:
ACK a7c96f874d
Tree-SHA512: 7c185838abaf068bc96b425c3c971b73f75dfcb41dacc8b2f2543c7602f23f19d908633278b93738f18049e6bd8c845c152cfb93b289bef501c7e86ed8dae0ab
9d5021a05b script: add SCRIPT_ERR_TAPSCRIPT_EMPTY_PUBKEY (billymcbip)
Pull request description:
We currently have two callsites for `SCRIPT_ERR_PUBKEYTYPE`:
- A pre-tapscript policy error behind the `SCRIPT_VERIFY_STRICTENC` flag: 4de26b111f/src/script/interpreter.cpp (L220)
- A [consensus error](https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki?plain=1#L93) in Tapscript: 4de26b111f/src/script/interpreter.cpp (L368)
It would be good for readability and testability to have separate errors for both cases, as they are quite distinct (policy vs. consensus, format vs. emptiness).
**This PR adds `SCRIPT_ERR_TAPSCRIPT_EMPTY_PUBKEY` for the consensus error path.**
This change would make our error handling more consistent. We have more granular errors for other pubkey error paths already: `SCRIPT_ERR_WITNESS_PUBKEYTYPE`, `SCRIPT_ERR_DISCOURAGE_UPGRADABLE_PUBKEYTYPE`. We also have separate errors for MINIMAL_IF: `SCRIPT_ERR_MINIMALIF` for the policy error pre-tapscript, and `SCRIPT_ERR_TAPSCRIPT_MINIMALIF` for the consensus error post-tapscript.
Tests:
Added a test case to `script_tests` and ran `build/bin/test_bitcoin --run_test=script_tests --log_level=success`.
```
test/script_tests.cpp:144: info: check '[["aa","#SCRIPT# 0 CHECKSIG","#CONTROLBLOCK#",0.00000001],"","0x51 0x20 #TAPROOTOUTPUT#","P2SH,WITNESS,TAPROOT","TAPSCRIPT_EMPTY_PUBKEY","TAPSCRIPT: OP_CHECKSIG with empty pubkey must fail"] (with flags 165d5d)' has passed
...
```
Ran `DIR_UNIT_TEST_DATA="$(pwd)/../qa-assets/unit_test_data" build/bin/test_bitcoin --run_test=script_assets_tests --log_level=success`.
Updated `feature_taproot.py` and ran `build/test/functional/feature_taproot.py`.
Looking forward to your feedback.
ACKs for top commit:
sedited:
ACK 9d5021a05b
darosior:
utACK 9d5021a05b
sipa:
ACK 9d5021a05b
Tree-SHA512: bc0b7f64454313fe392ffb2d23aa4eca3deadc5ea1d10b3fba0b3ab4cb0575a5ddcb002dc27b4fa7aa3c180840a83d1b3e5c89351009ce7ffe684d58e1980ace
"tor" as a network specification was deprecated in 60dc8e4208 in favor
of "onion" and this commit removes it and updates the relevant test.
Co-authored-by: Mara van der Laan <126646+laanwj@users.noreply.github.com>
As pointed out by Sjors in
https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598844209 and
https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598858386 the
original intention of having waitNext and waitTipChanged calls in the test was
to ensure that if new blocks were connected or fees were increased *during* the
waits, that the calls would wake up and return.
But the tests were written incorrectly, to generate blocks and transactions
before the wait calls instead of during the calls. So the tests were less
meaningful then they should be.
There was also a similar problem in the interruptWait test. The test was
intended to test the interruptWait method, but it was never actually calling
the method due to a missing await keyword. Instead it was testing that
miniwallet.send_self_transfer would interrupt the wait.
This commit fixes these issues by introducing a wait_and_do() helper function
to start parallel tasks and trigger an action after a wait call is started.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
There are a few things that are incorrect or messy in the interface_ipc.py test.
This commit tries to clean them up:
- isTestChain and isInitialBlockDownload asserts were not checking the results
of those calls, only that calls were, made because they were not checking the
responses' .result member.
- A lot of result accesses like `template.result` `mining.result` were repeated
unnecessarily because variables like `template` and `mining` were assigned
response objects instead of result objects. These variables are now changed
to point directly to results.
- Some coroutine calls were assigned to temporary `wait` before being awaited.
This was unnecessarily confusing and would make code not run in top-down
order.
- `to_dict` calls were being made to check if result variables were unset. This
was inefficient and indirect because it iterates over all fields in response
structs instead of just checking whether the result field is present. The
to_dict calls are now replaced with more direct `_has('result')` calls.
- The `res` variables used to hold various responses did not have descriptive
names. These are replaced with clearer names.
Co-authored-by: rkrux <rkrux.connect@gmail.com>
dcd42d6d8f [test] wallet send 3 generation TRUC (glozow)
e753fadfd0 [wallet] never try to spend from unconfirmed TRUC that already has ancestors (glozow)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3319935660
There is not an explicit check that the to-be-created wallet transaction would be within the {TRUC, normal} ancestor limits. This means that the wallet may create a transaction that violates these limits, but fail to broadcast it in `CommitTransaction`.
This appears to be expected behavior for the normal ancestor limits (and any other situation in which the wallet creates a tx that was rejected by mempool) and AFAIK the transaction will be rebroadcast at some point after the ancestors confirm.
1ed00a0d39/test/functional/wallet_basic.py (L502-L506)
It's a bit complex to address this for the normal ancestor limit, and probably unrealistic for the wallet to check all possible mempool policies in coin selection, but it's quite trivial for TRUC: just skip any unconfirmed UTXOs that have any ancestors. I think it would be much more helpful to the user to say there are insufficient funds.
ACKs for top commit:
achow101:
ACK dcd42d6d8f
monlovesmango:
ACK dcd42d6d8f
rkrux:
lgtm ACK dcd42d6d8f
Tree-SHA512: b4cf9685bf0593c356dc0d6644835d53e3d7089f42b65f647795257dc7f5dac90c5ee493b41ee30a1c1beb880a859db8e049d3c64a43d5ca9b3e6482ff6bddd5
Use context managers to destroy block templates. Previously, block templates
were not being destroyed before disconnecting because the destroy coroutines
were called but never awaited. It's not necessary to explicitly destroy the
templates since they will get garbage collected asynchronously, but it's good
to destroy them to make the test more predictable, and to make the destroy
calls that are present actually do something.
This change also removes `await waitnext` expressions without changing
behavior, because the previous code was misleading about what order waitNext
calls were executed.
This change is easiest to review ignoring whitespace.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
fa6db79302 test: Avoid shutdown race in NetworkThread (MarcoFalke)
Pull request description:
Locally, I am seeing rare intermittent exceptions in the network thread:
```
stderr:
Exception in thread NetworkThread:
Traceback (most recent call last):
File "/python3.10/threading.py", line 1016, in _bootstrap_inner
self.run()
File "./test/functional/test_framework/p2p.py", line 744, in run
self.network_event_loop.run_forever()
File "/python3.10/asyncio/base_events.py", line 603, in run_forever
self._run_once()
File "/python3.10/asyncio/base_events.py", line 1871, in _run_once
event_list = self._selector.select(timeout)
AttributeError: 'NoneType' object has no attribute 'select'
```
I can reproduce this intermittently via `while ./bld-cmake/test/functional/test_runner.py $(for i in {1..400}; do echo -n "tool_rpcauth "; done) -j 400 ; do true ; done`.
I suspect this is a race where the shutdown starts the close of the network thread while it is starting.
A different exception showing this race can be reproduced via:
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 610aa4ccca..64561e157c 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -741,6 +741,7 @@ class NetworkThread(threading.Thread):
def run(self):
"""Start the network thread."""
+ import time;time.sleep(.1)
self.network_event_loop.run_forever()
def close(self, *, timeout=10):
```
It is trivial to reproduce via any test (e.g. `./bld-cmake/test/functional/tool_rpcauth.py`) and shows a similar traceback to the one above:
```
Exception in thread NetworkThread:
Traceback (most recent call last):
File "/python3.10/threading.py", line 1016, in _bootstrap_inner
self.run()
File "./test/functional/test_framework/p2p.py", line 745, in run
self.network_event_loop.run_forever()
File "/python3.10/asyncio/base_events.py", line 591, in run_forever
self._check_closed()
File "/python3.10/asyncio/base_events.py", line 515, in _check_closed
raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed
```
So fix the second runtime error in hope of fixing the first one as well.
ACKs for top commit:
brunoerg:
code review ACK fa6db79302
Tree-SHA512: ca352ebf7929456ea2bbfcfe4f726adcbfcfb3dc0edeaddae7f6926f998888f0bd8b987ddef60308266eeab6bffa7ebdc32f5908db9de5404df95635dae4a8f6
cb7d5bfe4a test, assumeutxo: loading a wallet (backup) on a pruned node (Alfonso Roman Zubeldia)
7a365244f8 test, refactor snapshot import and background validation (Alfonso Roman Zubeldia)
Pull request description:
Adding tests in `./test/functional/wallet_assumeutxo.py` to cover the following scenario:
- test loading a wallet (backup) on a pruned node
ACKs for top commit:
fjahr:
re-ACK cb7d5bfe4a
theStack:
re-ACK cb7d5bfe4a
Tree-SHA512: 88cc419f340d31e80120e0c6cafe567efc678df27576db6e08aeab62d2b50ed1153d56f3f3343e9bae49262e38f9fb81db7769f02a4a01e4ef25c5d029c12323
fad6118586 test: Fix "typo" in written invalid content (MarcoFalke)
fab085c15f contrib: Use text=True in subprocess over manual encoding handling (MarcoFalke)
fa71c15f86 scripted-diff: Bump copyright headers after encoding changes (MarcoFalke)
fae612424b contrib: Remove confusing and redundant encoding from IO (MarcoFalke)
fa7d72bd1b lint: Drop check to enforce encoding to be specified in Python scripts (MarcoFalke)
faf39d8539 test: Clarify that Python UTF-8 mode is the default today for most systems (MarcoFalke)
fa83e3a81d lint: Do not allow locale dependent shell scripts (MarcoFalke)
Pull request description:
Historically, there was an attempt via `test/lint/lint-python-utf8-encoding.py` to enforce explicit UTF8 in every Python IO statement (`open`, `subprocess`, ...). However, the lint check has many problems:
* The check is incomplete and many IO statements lack the explicit UTF8 specification.
* It was added at a time when some systems were not UTF8 by default.
* The check is brittle, as it depends on a fragile regex.
In theory, now that the minimum Python version is 3.10 (since commit 2123c94448), the check could be replaced by `PYTHONWARNDEFAULTENCODING=1` from https://docs.python.org/3/whatsnew/3.10.html#optional-encodingwarning-and-encoding-locale-option. However, this comes with many other problems:
* All our Python scripts already assume and require UTF8 to be set externally. On almost all modern systems, this is already the default. Some Windows versions do not have UTF8 by default and require `PYTHONUTF8=1` to be set for the tests to run already today (with or without the changes in this pull). Also, the CI and many other Bash scripts force UTF8 via `LC_ALL`. Finally, Python 3.15 will likely enable UTF8 on *all* systems by default, per https://peps.python.org/pep-0686/#abstract.
* So adding UTF8 to every single IO call is redundant, verbose, and confusing, given that it is the expected default.
So fix all issues, by:
* Removing the `test/lint/lint-python-utf8-encoding.py` check.
* Removing the encoding on the individual IO calls.
* Clarifying the existing docs around the existing UTF8 requirement and assumption.
Obviously, every IO call is still free to specify UTF8 or any other encoding explicitly, if there is a documented need for it in the future.
ACKs for top commit:
theStack:
re-ACK fad6118586
laanwj:
Re-ACK fad6118586
Tree-SHA512: 78025ea3508597d2299490347614f0ee3e4c66e3ba559ff50e498045a9c8bbd92f3a5ced18719d8fcebbd1e47bdbb56a0c85a5b73b425adb0ea4f02fe69c3149
The existing IBD disable was added at a time when CheckForkWarningConditions
did also sophisticated fork detection that could lead to false positives
during IBD (55ed3f1475).
The fork detection logic doesn't exist anymore
(since fa62304c97), so the IBD check is no
longer necessary.
Displaying the log at startup will help node operators diagnose the
problem better.
Also unify log message and alert warning text, since a long invalid chain
could be due to chainstate corruption or an actual consensus incompatibility
with peers. Previously the log assumed the former and the alert the latter.
fa45a1503e log: Use LogWarning for non-critical logs (MarcoFalke)
fa0018d011 log: Use LogError for fatal errors (MarcoFalke)
22229de728 doc: Fix typo in init log (MarcoFalke)
Pull request description:
Logging supports severity levels above info via the legacy `LogPrintf`. So use the more appropriate `LogError` or `LogWarning`, where it applies.
This has a few small benefits:
* It often allows to remove the manual and literal "error: ", "Warning:", ... prefixes. Instead the uniform log level formatting is used.
* It is easier to grep or glance for more severe logs, which indicate some kind of alert.
* `LogPrintf` didn't indicate any severity level, but it is an alias for `LogInfo`. So having the log level explicitly spelled out makes it easier to read the code.
* Also, remove the redundant trailing `\n` newline, while touching.
* Also, remove the `__func__` formatting in the log string, which is redundant with `-logsourcelocations`. Instead, use a unique log string for each location.
ACKs for top commit:
l0rinc:
Code review ACK fa45a1503e
stickies-v:
ACK fa45a1503e
rkrux:
crACK fa45a1503e
Tree-SHA512: 516d439c36716f969c6e82d00bcda03c92c8765a9e41593b90052c86f8fa3a3dacbb2c3dc98bfc862cefa54cae34842b488671a20dd86cf1d15fb94aa5563406
b8d279a81c doc: add comment to explain correctness of GatherClusters() (Suhas Daftuar)
aba7500a30 Fix parameter name in getmempoolcluster rpc (Suhas Daftuar)
6c1325a091 Rename weight -> clusterweight in RPC output, and add doc explaining mempool terminology (Suhas Daftuar)
bc2eb931da Require mempool lock to be held when invoking TRUC checks (Suhas Daftuar)
957ae23241 Improve comments for getTransactionAncestry to reference cluster counts instead of descendants (Suhas Daftuar)
d97d6199ce Fix comment to reference cluster limits, not chain limits (Suhas Daftuar)
a1b341ef98 Sanity check feerate diagram in CTxMemPool::check() (Suhas Daftuar)
23d6f457c4 rpc: improve getmempoolcluster output (Suhas Daftuar)
d2dcd37aac Avoid using mapTx.modify() to update modified fees (Suhas Daftuar)
d84ffc24d2 doc: add release notes snippet for cluster mempool (Suhas Daftuar)
b0417ba944 doc: Add design notes for cluster mempool and explain new mempool limits (Suhas Daftuar)
2d88966e43 miner: replace "package" with "chunk" (Suhas Daftuar)
6f3e8eb300 Add a GetFeePerVSize() accessor to CFeeRate, and use it in the BlockAssembler (Suhas Daftuar)
b5f245f6f2 Remove unused DEFAULT_ANCESTOR_SIZE_LIMIT_KVB and DEFAULT_DESCENDANT_SIZE_LIMIT_KVB (Suhas Daftuar)
1dac54d506 Use cluster size limit instead of ancestor size limit in txpackage unit test (Suhas Daftuar)
04f65488ca Use cluster size limit instead of ancestor/descendant size limits when sanity checking TRUC policy limits (Suhas Daftuar)
634291a7dc Use cluster limits instead of ancestor/descendant limits when sanity checking package policy limits (Suhas Daftuar)
fc18ef1f3f Remove ancestor and descendant vsize limits from MemPoolLimits (Suhas Daftuar)
ed8e819121 Warn user if using -limitancestorsize/-limitdescendantsize that the options have no effect (Suhas Daftuar)
80d8df2d47 Invoke removeUnchecked() directly in removeForBlock() (Suhas Daftuar)
9292570f4c Rewrite GetChildren without sets (Suhas Daftuar)
3e39ea8c30 Rewrite removeForReorg to avoid using sets (Suhas Daftuar)
a3c31dfd71 scripted-diff: rename AddToMempool -> TryAddToMempool (Suhas Daftuar)
a5a7905d83 Simplify removeRecursive (Suhas Daftuar)
01d8520038 Remove unused argument to RemoveStaged (Suhas Daftuar)
bc64013e6f Remove unused variable (cacheMap) in mempool (Suhas Daftuar)
Pull request description:
As suggested in the main cluster mempool PR (https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-3177119367), I've pulled out some of the non-essential optimizations and cleanups into this separate PR.
Will continue to add more commits here to address non-blocking suggestions/improvements as they come up.
ACKs for top commit:
instagibbs:
ACK b8d279a81c
sipa:
ACK b8d279a81c
Tree-SHA512: 1a05e99eaf8db2e274a1801307fed5d82f8f917e75ccb9ab0e1b0eb2f9672b13c79d691d78ea7cd96900d0e7d5031a3dd582ebcccc9b1d66eb7455b1d3642235
Also improve test coverage for removeForReorg by creating a scenario where
there are in-mempool descendants that are only invalidated due to an in-mempool
parent no longer spending a mature coin.
Empty public keys in tapscript are rejected by consensus rules, independent of SCRIPT_VERIFY_STRICTENC. Add SCRIPT_ERR_TAPSCRIPT_EMPTY_PUBKEY to distinguish this from STRICTENC policy failures currently reported as SCRIPT_ERR_PUBKEYTYPE.
All touched Python scripts already assume and require UTF8, so manually
specifying encoding or decoding for functions in the subprocess module
is redundant to just using text=True, which exists since Python 3.7
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
The encoding arg is confusing, because it is not applied consistently
for all IO.
Also, it is useless, as the majority of files are ASCII encoded, which
are fine to encode and decode with any mode.
Moreover, UTF-8 is already required for most scripts to work properly,
so setting the encoding twice is redundant.
So remove the encoding from most IO. It would be fine to remove from all
IO, however I kept it for two files:
* contrib/asmap/asmap-tool.py: This specifically looks for utf-8
encoding errors, so it makes sense to sepecify the utf-8 encoding
explicitly.
* test/functional/test_framework/test_node.py: Reading the debug log in
text mode specifically counts the utf-8 characters (not bytes), so it
makes sense to specify the utf-8 encoding explicitly.
The check was incomplete and brittle. A better check would be to enable
`PYTHONWARNDEFAULTENCODING=1`
https://docs.python.org/3/whatsnew/3.10.html#optional-encodingwarning-and-encoding-locale-option
However, it is unclear what the goal of adding explicit encodings
everywhere is, given that:
* Most modern systems already have UTF-8 enabled by default, except for
Windows.
* Python 3.15 will likely enable it globally by default, according to
https://peps.python.org/pep-0686/#abstract
* Adding the explicit encodings will bloat all code for no benefit.
So remove the lint check and drop all redundant encoding= kwargs.
All encoding= that are set for a reason, are kept.
It will likely be the default for all systems, starting with Python
3.15, according to https://peps.python.org/pep-0686/#abstract.
It is hard to find a system other than Windows that has it not enabled
today. Nonetheless, Bitcoin Core requires UTF-8 in scripts and normally
enforces it via LC_ALL=C.UTF-8 or PYTHONUTF8=1.
Bash is discouraged, and there was never a need to write locale
dependent Bash.
So remove the option and clarify that the LC_ALL settings enable UTF-8
mode in Python.
8343a9ffcc test: add `-alertnotify` test for large work invalid chain warning (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `LARGE_WORK_INVALID_CHAIN` fork warning, checked with the `-alertnotify` option:
ead849c9f1/src/validation.cpp (L2033-L2040)
Found that this is missing during review of #32587. The test works by first creating a bunch of invalid blocks, that are first announced by headers and then submitted fully in reverse (invalid tip first), in order to set `m_best_invalid` to that value, finally leading to the best chain / invalid chain gap of >= 6 blocks. I'd be curious if there are other (more realistic?) ways to test this. One simple alternative is just to call `invalidateblock` twice (once at the tip, once at the base of the invalid chain).
Note that the written warning doesn't include the exclamation mark, as it is removed via `SanitizeString` in the `AlertNotify` function.
ACKs for top commit:
brunoerg:
reACK 8343a9ffcc
mzumsande:
re-ACK 8343a9ffcc
Tree-SHA512: d81e9ce7622026498cad5cdcdb867a22068670983737502888c72c72209ca6ff183e77d7429f758765a42c25cda439e01f795884864ac6fe6ff258a98d0bbcbc
17cf9ff7ef Use cluster size limit for -maxmempool bound, and allow -maxmempool=0 in general (Suhas Daftuar)
315e43e5d8 Sanity check `GetFeerateDiagram()` in CTxMemPool::check() (Suhas Daftuar)
de2e9a24c4 test: extend package rbf functional test to larger clusters (Suhas Daftuar)
4ef4ddb504 doc: update policy/packages.md for new package acceptance logic (Suhas Daftuar)
79f73ad713 Add check that GetSortedScoreWithTopology() agrees with CompareMiningScoreWithTopology() (Suhas Daftuar)
a86ac11768 Update comments for CTxMemPool class (Suhas Daftuar)
9567eaa66d Invoke TxGraph::DoWork() at appropriate times (Suhas Daftuar)
6c5c44f774 test: add functional test for new cluster mempool RPCs (Suhas Daftuar)
72f60c877e doc: Update mempool_replacements.md to reflect feerate diagram checks (Suhas Daftuar)
21693f031a Expose cluster information via rpc (Suhas Daftuar)
72e74e0d42 fuzz: try to add more code coverage for mempool fuzzing (Suhas Daftuar)
f107417490 bench: add more mempool benchmarks (Suhas Daftuar)
7976eb1ae7 Avoid violating mempool policy limits in tests (Suhas Daftuar)
84de685cf7 Stop tracking parents/children outside of txgraph (Suhas Daftuar)
88672e205b Rewrite GatherClusters to use the txgraph implementation (Suhas Daftuar)
1ca4f01090 Fix miniminer_tests to work with cluster limits (Suhas Daftuar)
1902111e0f Eliminate CheckPackageLimits, which no longer does anything (Suhas Daftuar)
3a646ec462 Rework RBF and TRUC validation (Suhas Daftuar)
19b8479868 Make getting parents/children a function of the mempool, not a mempool entry (Suhas Daftuar)
5560913e51 Rework truc_policy to use descendants, not children (Suhas Daftuar)
a4458d6c40 Use txgraph to calculate descendants (Suhas Daftuar)
c8b6f70d64 Use txgraph to calculate ancestors (Suhas Daftuar)
241a3e666b Simplify ancestor calculation functions (Suhas Daftuar)
b9cec7f0a1 Make removeConflicts private (Suhas Daftuar)
0402e6c780 Remove unused limits from CalculateMemPoolAncestors (Suhas Daftuar)
08be765ac2 Remove mempool logic designed to maintain ancestor/descendant state (Suhas Daftuar)
fc4e3e6bc1 Remove unused members from CTxMemPoolEntry (Suhas Daftuar)
ff3b398d12 mempool: eliminate accessors to mempool entry ancestor/descendant cached state (Suhas Daftuar)
b9a2039f51 Eliminate use of cached ancestor data in miniminer_tests and truc_policy (Suhas Daftuar)
ba09fc9774 mempool: Remove unused function CalculateDescendantMaximum (Suhas Daftuar)
8e49477e86 wallet: Replace max descendant count with cluster_count (Suhas Daftuar)
e031085fd4 Eliminate Single-Conflict RBF Carve Out (Suhas Daftuar)
cf3ab8e1d0 Stop enforcing descendant size/count limits (Suhas Daftuar)
89ae38f489 test: remove rbf carveout test from mempool_limit.py (Suhas Daftuar)
c0bd04d18f Calculate descendant information for mempool RPC output on-the-fly (Suhas Daftuar)
bdcefb8a8b Use mempool/txgraph to determine if a tx has descendants (Suhas Daftuar)
69e1eaa6ed Add test case for cluster size limits to TRUC logic (Suhas Daftuar)
9cda64b86c Stop enforcing ancestor size/count limits (Suhas Daftuar)
1f93227a84 Remove dependency on cached ancestor data in mini-miner (Suhas Daftuar)
9fbe0a4ac2 rpc: Calculate ancestor data from scratch for mempool rpc calls (Suhas Daftuar)
7961496dda Reimplement GetTransactionAncestry() to not rely on cached data (Suhas Daftuar)
feceaa42e8 Remove CTxMemPool::GetSortedDepthAndScore (Suhas Daftuar)
21b5cea588 Use cluster linearization for transaction relay sort order (Suhas Daftuar)
6445aa7d97 Remove the ancestor and descendant indices from the mempool (Suhas Daftuar)
216e693729 Implement new RBF logic for cluster mempool (Suhas Daftuar)
ff8f115dec policy: Remove CPFP carveout rule (Suhas Daftuar)
c3f1afc934 test: rewrite PopulateMempool to not violate mempool policy (cluster size) limits (Suhas Daftuar)
47ab32fdb1 Select transactions for blocks based on chunk feerate (Suhas Daftuar)
dec138d1dd fuzz: remove comparison between mini_miner block construction and miner (Suhas Daftuar)
6c2bceb200 bench: rewrite ComplexMemPool to not create oversized clusters (Suhas Daftuar)
1ad4590f63 Limit mempool size based on chunk feerate (Suhas Daftuar)
b11c89cab2 Rework miner_tests to not require large cluster limit (Suhas Daftuar)
95a8297d48 Check cluster limits when using -walletrejectlongchains (Suhas Daftuar)
95762e6759 Do not allow mempool clusters to exceed configured limits (Suhas Daftuar)
edb3e7cdf6 [test] rework/delete feature_rbf tests requiring large clusters (glozow)
435fd56711 test: update feature_rbf.py replacement test (Suhas Daftuar)
34e32985e8 Add new (unused) limits for cluster size/count (Suhas Daftuar)
838d7e3553 Add transactions to txgraph, but without cluster dependencies (Suhas Daftuar)
d5ed9cb3eb Add accessor for sigops-adjusted weight (Suhas Daftuar)
1bf3b51396 Add sigops adjusted weight calculator (Suhas Daftuar)
c18c68a950 Create a txgraph inside CTxMemPool (Suhas Daftuar)
29a94d5b2f Make CTxMemPoolEntry derive from TxGraph::Ref (Suhas Daftuar)
92b0079fe3 Allow moving CTxMemPoolEntry objects, disallow copying (Suhas Daftuar)
6c73e47448 mempool: Store iterators into mapTx in mapNextTx (Suhas Daftuar)
51430680ec Allow moving an Epoch::Marker (Suhas Daftuar)
Pull request description:
[Reopening #28676 here as a new PR, because GitHub is slow to load the page making it hard to scroll through and see comments. Also, that PR was originally opened with a prototype implementation which has changed significantly with the introduction of `TxGraph`.]
This is an implementation of the [cluster mempool proposal](https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393).
This branch implements the following observable behavior changes:
- Maintains a partitioning of the mempool into connected clusters (via the `txgraph` class), which are limited in vsize to 101 kvB by default, and limited in count to 64 by default.
- Each cluster is sorted ("linearized") to try to optimize for selecting highest-feerate-subsets of a cluster first
- Transaction selection for mining is updated to use the cluster linearizations, selecting highest feerate "chunks" first for inclusion in a block template.
- Mempool eviction is updated to use the cluster linearizations, selecting lowest feerate "chunks" first for removal.
- The RBF rules are updated to: (a) drop the requirement that no new inputs are introduced; (b) change the feerate requirement to instead check that the feerate diagram of the mempool will strictly improve; (c) replace the direct conflicts limit with a directly-conflicting-clusters limit.
- The CPFP carveout rule is eliminated (it doesn't make sense in a cluster-limited mempool)
- The ancestor and descendant limits are no longer enforced.
- New cluster count/cluster vsize limits are now enforced instead.
- Transaction relay now uses chunk feerate comparisons to determine the order that newly received transactions are announced to peers.
Additionally, the cached ancestor and descendant data are dropped from the mempool, along with the multi_index indices that were maintained to sort the mempool by ancestor and descendant feerates. For compatibility (eg with wallet behavior or RPCs exposing this), this information is now calculated dynamically instead.
ACKs for top commit:
instagibbs:
reACK 17cf9ff7ef
glozow:
reACK 17cf9ff7ef
sipa:
ACK 17cf9ff7ef
Tree-SHA512: bbde46d913d56f8d9c0426cb0a6c4fa80b01b0a4c2299500769921f886082fb4f51f1694e0ee1bc318c52e1976d7ebed8134a64eda0b8044f3a708c04938eee7
Previously we would sanity check the -maxmempool configuration based on a
multiple of the descendant size limit, but with cluster mempool the maximum
evicted size is now the cluster size limit, so use that instead.
Also allow -maxmempool=0 in general (and not just if
-limitdescendantsize/-limitclustersize is set to 0).
288b8c30be doc: Drop (default: none) from -i2psam description (Ryan Ofsky)
f6ec3519a3 init: Require explicit -asmap filename (Ryan Ofsky)
Pull request description:
Currently, if `-asmap` is specified without a filename bitcoind tries to load `ip_asn.map` data file.
This change now requires `-asmap=ip_asn.map` or another filename to be specified explicitly.
The change is intended to make behavior of the option explicit and avoid confusion reported https://github.com/bitcoin/bitcoin/issues/33386 where documentation specifies a default file which is not actually loaded by default. It was originally implemented in
https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3410302383 and various alternatives are discussed there.
ACKs for top commit:
brunoerg:
reACK 288b8c30be
fjahr:
re-ACK 288b8c30be
vostrnad:
utACK 288b8c30be
achow101:
ACK 288b8c30be
Tree-SHA512: 11a38a03892a58d6ccc1505cfbf915f58a86df9891761d89dc54b92d40593ee3cbb2d7c7bdbb922b871b3529072ef7f34cc98393aff6e8f0633b56352315b27c
de7c3587cd doc: Update add checksum instructions in tutorial (Ben Westgate)
2a46e94a16 doc: Update multisig-tutorial.md to use multipath descriptors (Ben Westgate)
Pull request description:
### Summary
Update `doc/multisig-tutorial.md` to use multipath descriptor format
instead of separate external/internal descriptors. The tutorial now:
- extracts a single `xpub_n` per participant
- constructs a multipath `wsh(sortedmulti(...))` descriptor with `<0;1>`
change index semantics
- uses `getdescriptorinfo` to compute descriptor checksum
- explains that `importdescriptors` expands the multipath descriptor
into internal and external descriptors
- update `/test/functional/wallet_multisig_descriptor_psbt.py` functional test / documentation to use multi-path descriptors
---
### Motivation
A single multipath descriptor is the most convenient pattern for multisig; our documentation should use it.
---
### What changed
- replaced extraction of `external_xpub_n` and `internal_xpub_n` with
extraction of a single `xpub_n`
- removed instructions to create and import separate external/internal
descriptors
- added instructions to build a multipath `wsh(sortedmulti(...))`
descriptor and derive checksum with `getdescriptorinfo`
- checksum field is parsed and appended as the multipath descriptor is not the canonical "desc" output
- clarified that `importdescriptors` automatically expands multipath
descriptors into internal and external forms
- similar changes to the functional test: wallet_multisig_descriptor_psbt.
---
### Testing
I have run the updated shell snippets and confirmed the multipath descriptor produces the same `listdescriptors` output after importing as the two descriptor method in bitcoin:master.
---
### Related issues / PRs
This tutorial change references the multipath descriptor
consolidation (see commit / PR referenced in the change). The commit
message points to bitcoin#22838 as the upstream change that enables
this behavior.
---
### Release note (for changelog)
Documentation: update multisig tutorial and multisig functional test to use multipath descriptors
ACKs for top commit:
Sjors:
utACK de7c3587cd
kannapoix:
Core review ACK: de7c3587cd
achow101:
ACK de7c3587cd
rkrux:
crACK de7c3587cd
Tree-SHA512: ca7275d8ad04922b3fa8d2a3084ff96aa2104265f1fc2f749814dd16776351ab692d67e7e643d08052f7492e3eaa1a9a4dfe3470163e57939a49f782d3df511a
