111465d72d test: Remove unused attempts parameter from wait_until (Fabian Jahr)
5468a23eb9 test: Add check_interval parameter to wait_until (Fabian Jahr)
16c87d91fd test: Introduce ensure_for helper (Fabian Jahr)
Pull request description:
A repeating pattern in the functional tests is that the test sleeps for a while to ensure that a certain condition is still true after some amount of time has elapsed. Most recently a new case of this was added in #30807. This PR here introduces an `ensure` helper to streamline this functionality.
Some approach considerations:
- It is possible to construct this by reusing `wait_until` and wrapping it in `try` internally. However, the logger output of the failing wait would still be printed which seems irritating. So I opted for simplified but similar internals to `wait_until`.
- This implementation starts for a failure in the condition right away which has the nice side-effect that it might give feedback on a failure earlier than is currently the case. However, in some cases, it may be expected that the condition may still be false at the beginning and then turns true until time has run out, something that would work when the test sleeps without checking in a loop. I decided against this design (and even against adding it as an option) because such a test design seems like it would be racy either way.
- I have also been going back and forth on naming. To me `ensure` works well but I am also not a native speaker, happy consider a different name if others don't think it's clear enough.
ACKs for top commit:
maflcko:
re-ACK 111465d72d🍋
achow101:
ACK 111465d72d
tdb3:
code review re ACK 111465d72d
furszy:
utACK 111465d72d
Tree-SHA512: ce01a4f3531995375a6fbf01b27d51daa9d4c3d7cd10381be6e86ec5925d2965861000f7cb4796b8d40aabe3b64c4c27e2811270e4e3c9916689575b8ba4a2aa
This improves the error message of the getblock and getblockstats rpc and prevents calls to
ReadRawBlockFromDisk(), which are unnecessary if we know
from the header nStatus field that the block is not available.
Exercising and verifying the following points:
1. An IBD node can sync headers from an AssumeUTXO node at
any time.
2. IBD nodes do not request historical blocks from AssumeUTXO
nodes while they are syncing the background-chain.
3. The assumeUTXO node dynamically adjusts the network services
it offers according to its state.
4. IBD nodes can fully sync from AssumeUTXO nodes after they
finish the background-chain sync.
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local
address through the network before completing the background chain sync.
This, combined with the advertising of full-node service (NODE_NETWORK), can
result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing)
and requesting an historical block the node does not have. This behavior leads to
an abrupt disconnection due to perceived unresponsiveness (lack of response)
from the AssumeUTXO node.
This lack of response occurs because nodes ignore getdata requests when they do
not have the block data available (further discussion can be found in PR 30385).
Fix this by refraining from signaling full-node service support while the
background chain is being synced. During this period, the node will only
signal 'NODE_NETWORK_LIMITED' support. Then, full-node ('NODE_NETWORK')
support will be re-enabled once the background chain sync is completed.
The error messages should never happen in normal operation. However, if
they do, they are helpful to return to the user to debug the issue. For
example, to notice a truncated file.
d63ef73800 test: Add loadtxoutset test with tip on snapshot block (Fabian Jahr)
c2f86d4bcb test: Remove already resolved assumeutxo todo comments (Fabian Jahr)
Pull request description:
The first commit removes three Todos that have been addressed previously (see commit message for details).
The second message resolves another todo by adding the missing test case. This is a special case of "the tip has more work than the snapshot" where the tip is the same block as the snapshot base block.
Related to #28648.
ACKs for top commit:
jrakibi:
ACK [d63ef73](d63ef73800)
achow101:
ACK d63ef73800
maflcko:
ACK d63ef73800
alfonsoromanz:
Re ACK d63ef73800
Tree-SHA512: 8d5a25fc0b26531db3a9740132694138f2103b7b42eeb1d4a64095bfc901c1372e23601c0855c7def84c8a4e185d10611e4e830c4e479f1b663ae6ed53abb130
a8e3af1a82 qa: Do not assume running `feature_asmap.py` from source directory (Hennadii Stepanov)
9bf7ca6cad qa: Consider `cache` and `config.ini` relative to invocation directory (Hennadii Stepanov)
a0473442d1 scripted-diff: Add `__file__` argument to `BitcoinTestFramework.init()` (Hennadii Stepanov)
Pull request description:
This PR includes changes split from https://github.com/bitcoin/bitcoin/pull/30454. They improve the functional test framework, allowing users to [run individual functional tests](https://github.com/hebasto/bitcoin/issues/146) from the build directory in the new CMake-based build system.
This functionality is not available for out-of-source builds using the current Autotools-based build system, which always requires write permissions for the source directory. Nevertheless, this PR can be tested as suggested in https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232618421:
1. Make an out-of-source build:
```
$ ./autogen.sh
$ mkdir ../build && cd ../build
$ ../bitcoin/configure
$ make
```
2. Create a symlink in the build directory to a functional test:
```
$ ln --symbolic ../../../bitcoin/test/functional/wallet_disable.py ./test/functional/
```
3. Run this symlink:
```
$ ./test/functional/wallet_disable.py
```
The last command fails on the master branch:
```
Traceback (most recent call last):
File "/home/hebasto/git/build/./test/functional/wallet_disable.py", line 31, in <module>
DisableWalletTest().main()
^^^^^^^^^^^^^^^^^^^
File "/home/hebasto/git/bitcoin/test/functional/test_framework/test_framework.py", line 106, in __init__
self.parse_args()
File "/home/hebasto/git/bitcoin/test/functional/test_framework/test_framework.py", line 210, in parse_args
config.read_file(open(self.options.configfile))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '/home/hebasto/git/bitcoin/test/config.ini'
```
and succeeds with this PR.
ACKs for top commit:
maflcko:
tested ACK a8e3af1a82🎨
glozow:
ACK a8e3af1a82, tested with the steps in op
stickies-v:
ACK a8e3af1a82
Tree-SHA512: 899e4efc09edec13ea3f5b47825d03173fb21d3569c360deda7fa6a56b99b4d24e09ad4f0883bad1ee926b1c706e47ba07c6a6160c63c07c82b3cf4ae5816e91
Also pulls out the guarding assert and calls it explicitly before the test function is called. This is already done before the existing call of the test function so it was not needed there.
- "Valid snapshot file, but referencing a snapshot block that turns out
to be invalid, or has an invalid parent" has been addressed in #30267
- "An ancestor of snapshot block" - If chain tip refers to blocks in this context then any successful load is addressing this because if we have synced past the snapshot base block we fail because we don't need assumeutxo anymore. And if this is about headers then this is the `test_headers_not_synced()` case.
- "A descendant of the snapshot block" - If this refers to blocks the
`test_snapshot_with_less_work()` addressed this and if it is just headers in this case again it would be represented in all of the successful loads in the test.
Co-authored-by: Alfonso Roman Zubeldia <alfonsoromanz24@gmail.com>
If the snapshot is not an ancestor of the most-work header (m_best_header),
syncing from that alternative chain should be prioritised.
Therefore don't accept loading a snapshot in this situation.
If that other chain turns out to be invalid, m_best_header
would be reset and loading the snapshot should be possible again.
Because of the work required to generate a conflicting headers chain,
this should only be possible under extreme circumstances, such as major forks.
5b7f70ba26 test: loadtxoutset in divergent chain with less work (Alfonso Roman Zubeldia)
d35efe1efc p2p: Start downloading historical blocks from common ancestor (Martin Zumsande)
Pull request description:
This PR adds a test to cover the scenario of loading an assumeutxo snapshot when the current chain tip is not an ancestor of the snapshot block but has less work.
During the review process, a bug was discovered where blocks between the last common ancestor and the background tip were not being requested if the background tip was not an ancestor of the snapshot block. mzumsande suggested a fix (65343ec49a6b73c4197dfc38e1c2f433b0a3838a) to start downloading historical blocks from the last common ancestor to address this issue. This fix has been incorporated into the PR with a slight modification.
Related to https://github.com/bitcoin/bitcoin/issues/28648
ACKs for top commit:
fjahr:
tACK 5b7f70ba26
achow101:
ACK 5b7f70ba26
mzumsande:
Code Review ACK 5b7f70ba26
Tree-SHA512: f8957349686a6a1292165ea9e0fd8c912d21466072632a10f8ef9d852a5f430bc6b2a531e6884a4dbf2e3adb28b3d512b25919e78f5804a67320ef54c3b1aaf6
Add the possibility of giving more guidance to the creation of the
metadata and/or coins, so that the fuzzer gets the chance
to reach more error conditions in ActivateSnapshot and sometimes
successfully creates a valid snapshot.
This also changes the asserts for the success case that were outdated,
and only didn't result in a crash because the fuzzer wasn't able
to reach this code before.
2342b46c45 test: Add coverage for getchaintxstats in assumeutxo context (Fabian Jahr)
faf2a6750b rpc: Reorder getchaintxstats output (MarcoFalke)
fa2dada0c9 rpc: Avoid getchaintxstats invalid results (MarcoFalke)
Pull request description:
The `getchaintxstats` RPC reply during AU background download may return non-zero, but invalid, values for `window_tx_count` and `txrate`.
For example, `txcount` may be zero for a to-be-downloaded block, but may be non-zero for an ancestor block which is already downloaded. Thus, the values returned may be negative (and cause intermediate integer sanitizer violations).
Also, `txcount` may be accurate for the snapshot base block, or a descendant of it. However it may be zero for an ancestor block that still needs to be downloaded. Thus, the values returned may be positive, but wrong.
Fix all issues by skipping the returned value if either `txcount` is unset (equal to zero).
Also, skip `txcount` in the returned value, if it is unset (equal to zero).
Fixes https://github.com/bitcoin/bitcoin/issues/29328
ACKs for top commit:
fjahr:
re-ACK 2342b46c45
achow101:
ACK 2342b46c45
mzumsande:
ACK 2342b46c45
Tree-SHA512: 931cecc40ee5dc0f96be728db7eb297155f8343076cd29c8b8c050c99fd1d568b80f54c9459a34ca7a9489c2474c729796d00eeb1934d6a9f7b4d6a53e3ec430
df6dc2aaae test: Assumeutxo: snapshots with less work should not be loaded (Hernan Marino)
Pull request description:
This PR adds a test which checks that snapshots with less accumulated work than the node's active chain, should not be loaded and return with an error. Although in a different context of discussion the missing test was detect in a thread in https://github.com/bitcoin/bitcoin/pull/29394 (see https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1484122214)
ACKs for top commit:
maflcko:
utACK df6dc2aaae
kevkevinpal:
utACK [df6dc2a](df6dc2aaae)
achow101:
ACK df6dc2aaae
alfonsoromanz:
Re ACK df6dc2aaae. Make is successful and the test passes.
Tree-SHA512: 07a394b4b288cc8ad3f66ed4e70dcda468db18113e9442eb7215cf491768432d55efaaa5b79d633094917e05475a30f0c5e4f64f8f2da293ba306891b4485560
542e13b293 rpc: Enhance metadata of the dumptxoutset output (Fabian Jahr)
4d8e5edbaa assumeutxo: Add documentation on dumptxoutset serialization format (Fabian Jahr)
c14ed7f384 assumeutxo: Add test for changed coin size value (Fabian Jahr)
de95953d87 rpc: Optimize serialization disk space of dumptxoutset (Fabian Jahr)
Pull request description:
The second attempt at implementing the `dumptxoutset` space optimization as suggested in #25675. Closes#25675.
This builds on the work done in #26045, addresses open feedback, adds some further improvements (most importantly usage of compact size), documentation, and an additional test.
The [original snapshot at height 830,000](https://github.com/bitcoin/bitcoin/pull/29551) came in at 10.82 GB. With this change, the same snapshot is 8.94 GB, a reduction of 17.4%.
This also enhances the metadata of the output file and adds the following data to allow for better error handling and make future upgrades easier:
- A newly introduced utxo set magic
- A version number
- The network magic
- The block height
ACKs for top commit:
achow101:
ACK 542e13b293
TheCharlatan:
Re-ACK 542e13b293
theStack:
ACK 542e13b293
Tree-SHA512: 0825d30e5c3c364062db3c6cbca4e3c680e6e6d3e259fa70c0c2b2a7020f24a47406a623582040988d5c7745b08649c31110df4c10656aa25f3f27eb35843d99
b259b0e8d3 [Test] Assumeutxo: ensure failure when importing a snapshot twice (Alfonso Roman Zubeldia)
Pull request description:
I am getting familiar with the `assume_utxo` tests and I found that the scenario of trying to activate a snapshot twice is not covered. This test is to ensure failure when loading a snapshot if there is already a snapshot-based chainstate.
ACKs for top commit:
fjahr:
Code review ACK b259b0e8d3
kevkevinpal:
tACK [b259b0e](b259b0e8d3)
achow101:
ACK b259b0e8d3
rkrux:
tACK [b259b0e](b259b0e8d3)
Tree-SHA512: 3510861390d0e40cdad6861b728df04827a1b63e642f3d956aee66ed2770b1cb7e3aa3eb00c62eb9da0544703c943cc5296936c9ebfcac18c719741c354421bb
ee67bba76c test: added test coverage to loadtxoutset (kevkevin)
Pull request description:
The functional test coverage did not cover the rpc error of "Couldn't open file..." for loadtxoutset and this test adds coverage for it
This adds coverage to this line
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L2777
ACKs for top commit:
maflcko:
ACK ee67bba76c
davidgumberg:
LGTM ACK ee67bba76c
rkrux:
ACK [ee67bba](ee67bba76c)
alfonsoromanz:
ACK ee67bba76c. Code looks good to me. I also ran `test/functional/feature_assumeutxo.py` to make sure all tests passes, including this one.
tdb3:
ACK for ee67bba76c
Tree-SHA512: 210a7eb928f625d2a8d9acb63ee83cb4aaec9c267e5a0c52ad219c2935466e2cdc68667e30ad29566e6060981587e5bec42805d296f6e60f9b3b13f3330575f2
You can use this tool to decode the utxo snapshot https://github.com/jrakibi/utxo-live
Here’s an overview of how it’s done:
The serialization forma for a UTXO in the snapshot is as follows:
1. Transaction ID (txid) - 32 bytes
2. Output Index (outnum)- 4 bytes
3. VARINT (code) - A varible-length integer encoding the height and whether the transaction is a coinbase. The format of this VARINT is (height << 1) | coinbase_flag.
4. VARINT (amount_v) - A variable-length integer that represents a compressed format of the output amount (in satoshis).
For the test cases mentioned:
* b"\x84\x58" - This value corresponds to a VARINT representing the height and coinbase flag. Once we decode this code, we can extract the height and coinbase using height = code_decoded >> 1 and coinbase = code_decoded & 0x01. In our case, with code_decoded = 728, it results in height = 364 and coinbase = 0.
* b"\xCA\xD2\x8F\x5A" - This byte sequence represents a compressed amount value. The decompression function takes this value and translates it into a full amount in satoshis. In our case, the decompression of this amount translates to a number larger than the maximum allowed value of coins (21 million BTC)
test:Validate UTXO snapshot with coin_height > base_height & amount > money_supply
test:Validate UTXO snapshot with coin_height > base_height & amount > money_supply
The extra `bilingual_str` argument of the fatal error notifications and
`node::AbortNode()` is often unused and when used usually contains the
same string as the message argument. It also seems to be confusing,
since it is not consistently used for errors requiring user action. For
example some assumeutxo fatal errors require the user to do something,
but are not translated.
So simplify the fatal error and abort node interfaces by only passing a
translated string. This slightly changes the fatal errors displayed to
the user.
Also de-duplicate the abort error log since it is repeated in noui.cpp.
Add a test for a CheckBlockIndex crash that would happen before previous
"assumeutxo: Get rid of faked nTx and nChainTx values" commit.
The crash was an assert failure in the (pindex->nChainTx == pindex->nTx +
prev_chain_tx) check that would previously happen if the snapshot block was
submitted after loading the snapshot and downloading a few blocks after the
snapshot. In that case ReceivedBlockTransactions() previously would overwrite
the nChainTx value of the submitted snapshot block with a fake value based on
the previous block, so the (pindex->nChainTx == pindex->nTx + prev_chain_tx)
check would later fail on the first block after the snapshot. This test was
originally posted by Martin Zumsande <mzumsande@gmail.com> in
https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1974096225
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Add a test for a CheckBlockIndex crash that would happen before previous
"assumeutxo: Get rid of faked nTx and nChainTx values" commit.
The crash was an assert failure in the (pindex->nChainTx == pindex->nTx +
prev_chain_tx) check that would previously happen if a snapshot was loaded, and
a block was submitted which forked from the chain before the snapshot block and
after the last downloaded background chain block. This block would not be
marked assumed-valid because it would not be an ancestor of the snapshot, and
it would have nTx set, nChainTx unset, and prev->nChainTx set with a fake
value, so the assert would fail. After the fix, prev->nChainTx is unset instead
of being set to a fake value, so the assert succeeds. This test was originally
posted by maflcko in
https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1918947945
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
The `PopulateAndValidateSnapshot` function introduced in
f6e2da5fb7 from #19806 has been setting fake
`nTx` and `nChainTx` values that can show up in RPC results (see #29328) and
make `CBlockIndex` state hard to reason about, because it is difficult to know
whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the
values are unknown, instead of faking them.
This commit fixes at least two assert failures in the (pindex->nChainTx ==
pindex->nTx + prev_chain_tx) check that would happen previously. Tests for
these failures are added separately in the next two commits.
Compatibility note: This change could result in -checkblockindex failures if a
snapshot was loaded by a previous version of Bitcoin Core and not fully
validated, because fake nTx values will have been saved to the block index. It
would be pretty easy to avoid these failures by adding some compatibility code
to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake
(when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a
little simpler not to worry about being compatible in this case.
