Age | Commit message (Collapse) | Author |
|
3153e7d779ac284f86e433af033d63f13f361b6f [fuzz] Add HeadersSyncState target (dergoegge)
53552affca381cdb5103ecdbcc7f3fb562e66ac4 [headerssync] Make m_commit_offset protected (dergoegge)
Pull request description:
This adds a fuzz target for the `HeadersSyncState` class.
I am unsure how well this is able to cover the logic since it is just processing unserialized CBlockHeaders straight from the fuzz input (headers are sometimes made continuous). However, it does manage to get to the redownload phase so i thought it is better then not having fuzzing at all.
It would also be nice to fuzz the p2p logic that is using `HeadersSyncState` (e.g. `TryLowWorkHeadersSync`, `IsContinuationOfLowWorkHeadersSync`) but that likely requires some more work (refactoring👻).
ACKs for top commit:
mzumsande:
ACK 3153e7d779ac284f86e433af033d63f13f361b6f
Tree-SHA512: 8a4630ceeeb30e4eeabaa8eb5491d98f0bf900efe7cda07384eaac9f2afaccfbcaa979cc1cc7f0b6ca297a8f5c17a7759f94809dd87eb87d35348d847c83e8ab
|
|
802cc1ef536e11944608fe9ab782d3e962037703 Deduplicate bitcoind and bitcoin-qt init code (Ryan Ofsky)
d172b5c6718b69200c8ad211fe709860081bd692 Add InitError(error, details) overload (Ryan Ofsky)
3db2874bd71d2391747b7385cabcbfef67218c4c Extend bilingual_str support for tinyformat (Ryan Ofsky)
c361df90b9fd34e50bbf1db43b866930e5498357 scripted-diff: Remove double newlines after some init errors (Ryan Ofsky)
Pull request description:
Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code reading config files and creating the datadir.
Noticed the duplicate code while reviewing #27073 and want to remove it because difference in bitcoin-qt and bitcoind behavior make it hard to evaluate changes like #27073
There are a few minor changes in behavior:
- In bitcoin-qt, when there is a problem reading the configuration file, the GUI error text has changed from "Error: Cannot parse configuration file:" to "Error reading configuration file:" to be consistent with bitcoind.
- In bitcoind, when there is a problem reading the settings.json file, the error text has changed from "Failed loading settings file" to "Settings file could not be read" to be consistent with bitcoin-qt.
- In bitcoind, when there is a problem writing the settings.json file, the error text has changed from "Failed saving settings file" to "Settings file could not be written" to be consistent with bitcoin-qt.
- In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read), there is an normal error dialog showing "Error: filesystem error: status: Permission denied [.../settings.json]", instead of an uncaught exception.
ACKs for top commit:
Sjors:
Light review ACK 802cc1ef536e11944608fe9ab782d3e962037703
TheCharlatan:
ACK 802cc1ef536e11944608fe9ab782d3e962037703
achow101:
ACK 802cc1ef536e11944608fe9ab782d3e962037703
Tree-SHA512: 9c78d277e9ed595fa8ce286b97d2806e1ec06ddbbe7bd3434bd9dd7b456faf8d989f71231e97311f36edb9caaec645a50c730bd7514b8e0fe6e6f7741b13d981
|
|
The following scenarios are covered:
1) 10 UTXO with the same script:
partial spends is enabled --> outputs must not be grouped.
2) 10 UTXO with the same script:
partial spends disabled --> outputs must be grouped.
3) 20 UTXO, 10 one from scriptA + 10 from scriptB:
a) if partial spends is enabled --> outputs must not be grouped.
b) if partial spends is not enabled --> 2 output groups expected (one per script).
3) Try to add a negative output (value - fee < 0):
a) if "positive_only" is enabled --> negative output must be skipped.
b) if "positive_only" is disabled --> negative output must be added.
4) Try to add a non-eligible UTXO (due not fulfilling the min depth target for
"not mine" UTXOs) --> it must not be added to any group
5) Try to add a non-eligible UTXO (due not fulfilling the min depth target for
"mine" UTXOs) --> it must not be added to any group
6) Surpass the 'OUTPUT_GROUP_MAX_ENTRIES' size and verify that a second partial
group gets created.
|
|
Previous bilingual_str tinyformat::format accepted bilingual format strings,
but not bilingual arguments. Extend it to accept both. This is useful when
embedding one translated string inside another translated string, for example:
`strprintf(_("Error: %s"), message)` which would fail previously if `message`
was a bilingual_str.
|
|
various improvements
511aa4f1c7508f15cab8d7e58007900ad6fd3d5d Add unit test for ChaCha20's new caching (Pieter Wuille)
fb243d25f754da8f01793b41e2d225b917f3e5d7 Improve test vectors for ChaCha20 (Pieter Wuille)
93aee8bbdad808b7009279b67470d496cc26b936 Inline ChaCha20 32-byte specific constants (Pieter Wuille)
62ec713961ade7b58e90c905395558a41e8a59f0 Only support 32-byte keys in ChaCha20{,Aligned} (Pieter Wuille)
f21994a02e1cc46d41995581b54222abc655be93 Use ChaCha20Aligned in MuHash3072 code (Pieter Wuille)
5d16f757639e2cc6e81db6e07bc1d5dd74abca6c Use ChaCha20 caching in FastRandomContext (Pieter Wuille)
38eaece67b1bc37b2f502348c5d7537480a34346 Add fuzz test for testing that ChaCha20 works as a stream (Pieter Wuille)
5f05b27841af0bed1b6e7de5f46ffe33e5919e4d Add xoroshiro128++ PRNG (Martin Leitner-Ankerl)
12ff72476ac0dbf8add736ad3fb5fad2eeab156c Make unrestricted ChaCha20 cipher not waste keystream bytes (Pieter Wuille)
6babf402130a8f3ef3058594750aeaa50b8f5044 Rename ChaCha20::Seek -> Seek64 to clarify multiple of 64 (Pieter Wuille)
e37bcaa0a6dbb334ab6e817efcb609ccee6edc39 Split ChaCha20 into aligned/unaligned variants (Pieter Wuille)
Pull request description:
This is an alternative to #25354 (by my benchmarking, somewhat faster), subsumes #25712, and adds additional test vectors.
It separates the multiple-of-64-bytes-only "core" logic (which becomes simpler) from a layer around which performs caching/slicing to support arbitrary byte amounts. Both have their uses (in particular, the MuHash3072 code can benefit from multiple-of-64-bytes assumptions), plus the separation results in more readable code. Also, since FastRandomContext effectively had its own (more naive) caching on top of ChaCha20, that can be dropped in favor of ChaCha20's new built-in caching.
I thought about rebasing #25712 on top of this, but the changes before are fairly extensive, so redid it instead.
ACKs for top commit:
ajtowns:
ut reACK 511aa4f1c7508f15cab8d7e58007900ad6fd3d5d
dhruv:
tACK crACK 511aa4f1c7
Tree-SHA512: 3aa80971322a93e780c75a8d35bd39da3a9ea570fbae4491eaf0c45242f5f670a24a592c50ad870d5fd09b9f88ec06e274e8aa3cefd9561d623c63f7198cf2c7
|
|
The fuzzer goes through a sequence of operations that get applied to both a
real stack of CCoinsViewCache objects, and to simulation data, comparing
the two at the end.
|
|
Xoroshiro128++ is a fast non-cryptographic random generator.
Reference implementation is available at https://prng.di.unimi.it/
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
|
|
|
|
coverage
9622fe64b8785430c71d4abc8637075026dc690c test: move coins result test to wallet_tests.cpp (furszy)
f69347d0588647ff9a4e986c7be987827a0417f4 test: extend and simplify availablecoins_tests (furszy)
212ccdf2c2b70d973b18ae78f0158ec5f0c3bbb4 wallet: AvailableCoins, add arg to include/skip locked coins (furszy)
Pull request description:
Negative PR with extended test coverage :).
1) Cleaned duplicated code and added coverage for the 'AvailableCoins' incremental result.
2) The class `AvailableCoinsTestingSetup` inside `availablecoins_tests.cpp` is a plain copy
of `ListCoinsTestingSetup` that is inside `wallet_tests.cpp`.
So, deleted the file and moved the `BasicOutputTypesTest` test case to `wallet_tests.cpp`.
3) Added arg to include/skip locked coins from the `AvailableCoins` result. This is needed for point (1) as otherwise the wallet will spend the coins that we recently created due its closeness to the recipient amount.
Note: this last point comes from #25659 where I'm using the same functionality to clean/speedup another flow as well.
ACKs for top commit:
achow101:
ACK 9622fe64b8785430c71d4abc8637075026dc690c
theStack:
ACK 9622fe64b8785430c71d4abc8637075026dc690c
aureleoules:
reACK 9622fe64b8785430c71d4abc8637075026dc690c, nice cleanup!
Tree-SHA512: 1ed9133120bfe8815455d1ad317bb0ff96e11a0cc34ee8098716ab9b001749168fa649212b2fa14b330c1686cb1f29039ff1f88ae306db68881b0428c038f388
|
|
`-sanity-check` option
f1e89597c803001ab9d5afd7e173184fe6886d1d test: Drop no longer required bench output redirection (Hennadii Stepanov)
4dbcdf26a301f54c60c85ceab1aaa4dae43f6aeb bench: Suppress output when running with `-sanity-check` option (Hennadii Stepanov)
Pull request description:
This change allows to simplify CI tests, and makes it easier to integrate the `bench_bitcoin` binary into CMake custom [targets](https://cmake.org/cmake/help/latest/command/add_custom_target.html) or [commands](https://cmake.org/cmake/help/latest/command/add_custom_command.html), as `COMMAND` does not support output redirection.
ACKs for top commit:
aureleoules:
tACK f1e89597c803001ab9d5afd7e173184fe6886d1d. Ran as expected and is more practical than using an output redirection.
Tree-SHA512: 29086d428cccedcfd031c0b4514213cbc1670e35f955e8fd35cee212bc6f9616cf9f20d0cb984495390c4ae2c50788ace616aea907d44e0d6a905b9dda1685d8
|
|
The class `AvailableCoinsTestingSetup` inside `availablecoins_tests.cpp` is a plain copy
of `ListCoinsTestingSetup` that is inside wallet_tests.cpp.
|
|
|
|
files share the same purpose, and we shouldn't have wallet code
inside the test directory.
This later is needed to use wallet util functions in the bench
and test binaries without be forced to duplicate them.
|
|
|
|
|
|
framework
3e9d0bea8deb61596c91ead997e9db83f5b0ff68 build: only run high priority benchmarks in 'make check' (furszy)
466b54bd4ab8227ff8c066a027a92791366a81c1 bench: surround main() execution with try/catch (furszy)
3da7cd2a762077fa81dc40832d556d8a3fd53674 bench: explicitly make all current benchmarks "high" priority (furszy)
05b8c76232dedf938740e8034c725ac16d32974a bench: add "priority level" to the benchmark framework (furszy)
f1593780b8e3b6adefee08b10d270c5c329f91fe bench: place benchmark implementation inside benchmark namespace (furszy)
Pull request description:
This is from today's meeting, a simple "priority level" for the benchmark framework.
Will allow us to run certain benchmarks while skip non-prioritized ones in `make check`.
By default, `bench_bitcoin` will run all the benchmarks. `make check`will only run the high priority ones,
and have marked all the existent benchmarks as "high priority" to retain the current behavior.
Could test it by modifying any benchmark priority to something different from "high", and
run `bench_bitcoin -priority-level=high` and/or `bench_bitcoin -priority-level=medium,low`
(the first command will skip the modified bench while the second one will include it).
Note: the second commit could be avoided by having a default arg value for the priority
level but.. an explicit set in every `BENCHMARK` macro call makes it less error-prone.
ACKs for top commit:
kouloumos:
re-ACK 3e9d0bea8deb61596c91ead997e9db83f5b0ff68
achow101:
ACK 3e9d0bea8deb61596c91ead997e9db83f5b0ff68
theStack:
re-ACK 3e9d0bea8deb61596c91ead997e9db83f5b0ff68
stickies-v:
re-ACK https://github.com/bitcoin/bitcoin/commit/3e9d0bea8deb61596c91ead997e9db83f5b0ff68
Tree-SHA512: ece59bf424c5fc1db335f84caa507476fb8ad8c6151880f1f8289562e17023aae5b5e7de03e8cbba6337bf09215f9be331e9ef51c791c43bce43f7446813b054
|
|
|
|
|
|
reindex results in recoverable blk file corruption
bcb0cacac28e98a39dc856c574a0872fe17059e9 reindex, log, test: fixes #21379 (mruddy)
Pull request description:
Fixes #21379.
The blocks/blk?????.dat files are mutated and become increasingly malformed, or corrupt, as a result of running the re-indexing process.
The mutations occur after the re-indexing process has finished, as new blocks are appended, but are a result of a re-indexing process miscalculation that lingers in the block manager's `m_blockfile_info` `nSize` data until node restart.
These additions to the blk files are non-fatal, but also not desirable.
That is, this is a form of data corruption that the reading code is lenient enough to process (it skips the extra bytes), but it adds some scary looking log messages as it encounters them.
The summary of the problem is that the re-index process double counts the size of the serialization header (magic message start bytes [4 bytes] + length [4 bytes] = 8 bytes) while calculating the blk data file size (both values already account for the serialization header's size, hence why it is over accounted).
This bug manifests itself in a few different ways, after re-indexing, when a new block from a peer is processed:
1. If the new block will not fit into the last blk file processed while re-indexing, while remaining under the 128MiB limit, then the blk file is flushed to disk and truncated to a size that is 8 greater than it should be. The truncation adds zero bytes (see `FlatFileSeq::Flush` and `TruncateFile`).
1. If the last blk file processed while re-indexing has logical space for the new block under the 128 MiB limit:
1. If the blk file was not already large enough to hold the new block, then the zeros are, in effect, added by `fseek` when the file is opened for writing. Eight zero bytes are added to the end of the last blk file just before the new block is written. This happens because the write offset is 8 too great due to the miscalculation. The result is 8 zero bytes between the end of the last block and the beginning of the next block's magic + length + block.
1. If the blk file was already large enough to hold the new block, then the current existing file contents remain in the 8 byte gap between the end of the last block and the beginning of the next block's magic + length + block. Commonly, when this occcurs, it is due to the blk file containing blocks that are not connected to the block tree during reindex and are thus left behind by the reindex process and later overwritten when new blocks are added. The orphaned blocks can be valid blocks, but due to the nature of concurrent block download, the parent may not have been retrieved and written by the time the node was previously shutdown.
ACKs for top commit:
LarryRuane:
tested code-review ACK bcb0cacac28e98a39dc856c574a0872fe17059e9
ryanofsky:
Code review ACK bcb0cacac28e98a39dc856c574a0872fe17059e9. This is a disturbing bug with an easy fix which seems well-worth merging.
mzumsande:
ACK bcb0cacac28e98a39dc856c574a0872fe17059e9 (reviewed code and did some testing, I agree that it fixes the bug).
w0xlt:
tACK https://github.com/bitcoin/bitcoin/pull/24858/commits/bcb0cacac28e98a39dc856c574a0872fe17059e9
Tree-SHA512: acc97927ea712916506772550451136b0f1e5404e92df24cc05e405bb09eb6fe7c3011af3dd34a7723c3db17fda657ae85fa314387e43833791e9169c0febe51
|
|
This means we don't need datetime in a --disable-wallet build, and it
isn't included in the kernel.
|
|
This leaves $(BITCOIN_INCLUDES) as internal dependencies, and gives
finer control over Boost includes.
|
|
Previously, this was crashing the wallet.
|
|
|
|
|
|
|
|
5474f5c356c5a17fbf6c84110cf83a5753cd0367 build: fix cleanup of test logs (fanquake)
Pull request description:
`make clean` currently looks for `test_name.cpp.log`, when it should be `test_name.log`, meaning .log files are left after running `make clean`.
Also fixes #21705. `make distcheck` seems to work fine after the logs files are properly cleaned up:
```bash
./autogen.sh && ./configure && make distcheck -j9
....
make[1]: Leaving directory '/home/ubuntu/bitcoin/bitcoin-23.99.0/_build/sub'
if test -d "bitcoin-23.99.0"; then find "bitcoin-23.99.0" -type d ! -perm -200 -exec chmod u+w {} ';' && rm -rf "bitcoin-23.99.0" || { sleep 5 && rm -rf "bitcoin-23.99.0"; }; else :; fi
=================================================
bitcoin-23.99.0 archives ready for distribution:
bitcoin-23.99.0.tar.gz
=================================================
```
Probably broken in #19385 / #24715.
Guix Build (x86_64):
```bash
c33306c2ae55bc0e037a1050bd0813fd7654f21fefd0e7df089a541118b629bc guix-build-5474f5c356c5/output/aarch64-linux-gnu/SHA256SUMS.part
f3cf5b8366e27155f3a369ab0d017074912506c43b4010054a72e5c3ae8cab2c guix-build-5474f5c356c5/output/aarch64-linux-gnu/bitcoin-5474f5c356c5-aarch64-linux-gnu-debug.tar.gz
48f618300f63533c50c31395959737103bb0279972b989cc5417adbf338a5c9f guix-build-5474f5c356c5/output/aarch64-linux-gnu/bitcoin-5474f5c356c5-aarch64-linux-gnu.tar.gz
6b3e0ceefc84dfad48aec3a9ea8ae98a427775242370234709605855f593dc88 guix-build-5474f5c356c5/output/arm-linux-gnueabihf/SHA256SUMS.part
36a38f4d0d2d0fee51256ee9c610cde014055cf18b5b852c8b3235ef218b461e guix-build-5474f5c356c5/output/arm-linux-gnueabihf/bitcoin-5474f5c356c5-arm-linux-gnueabihf-debug.tar.gz
3acb46a786323068fe34eaa8b2f7bff428d35367677e456b78fc00db09da8adf guix-build-5474f5c356c5/output/arm-linux-gnueabihf/bitcoin-5474f5c356c5-arm-linux-gnueabihf.tar.gz
aaeb126cca3cbbb1d21be2cbb36b3e16a8c110199a5e61172ee55c9c913fb717 guix-build-5474f5c356c5/output/arm64-apple-darwin/SHA256SUMS.part
0135d47c34dfbfbcd68181292fc810a1be4efaa0ff9fab05f9f33415167fc82c guix-build-5474f5c356c5/output/arm64-apple-darwin/bitcoin-5474f5c356c5-arm64-apple-darwin-unsigned.dmg
261ae9f70238ecbd8fedf9b3e02f34601abe42c4a23b25d0571d04d0881fdaba guix-build-5474f5c356c5/output/arm64-apple-darwin/bitcoin-5474f5c356c5-arm64-apple-darwin-unsigned.tar.gz
de716e0f425bae38cfda2690e86dbfa6831b6d37dd8a7638ca14334eecbddf99 guix-build-5474f5c356c5/output/arm64-apple-darwin/bitcoin-5474f5c356c5-arm64-apple-darwin.tar.gz
2e2e1d5cd009ec5c69ce22124afe90ff8c15e9cb546df818b2305ff96efa9c81 guix-build-5474f5c356c5/output/dist-archive/bitcoin-5474f5c356c5.tar.gz
85dd41584a2c7715b16508dff0f51bbb20b3891a1901a1781ecc37bc1cfd97c8 guix-build-5474f5c356c5/output/powerpc64-linux-gnu/SHA256SUMS.part
49ae951e9acb34fede0ead4d53679ddc041bfb6d60646efaca2a99cf87972be9 guix-build-5474f5c356c5/output/powerpc64-linux-gnu/bitcoin-5474f5c356c5-powerpc64-linux-gnu-debug.tar.gz
2efcb3fbb6702bff30bbf05d83b9849f390c67a1c363c883d71f365f4bee7ef6 guix-build-5474f5c356c5/output/powerpc64-linux-gnu/bitcoin-5474f5c356c5-powerpc64-linux-gnu.tar.gz
cea9e8e2499932ef6e20bba7a6b3408e2cca6fcd1875c1890293dd745add6942 guix-build-5474f5c356c5/output/powerpc64le-linux-gnu/SHA256SUMS.part
7616659242a1f15b7d6f9f54382dfb52d0bbdca701e1fb3d48fbe7bb590e6213 guix-build-5474f5c356c5/output/powerpc64le-linux-gnu/bitcoin-5474f5c356c5-powerpc64le-linux-gnu-debug.tar.gz
d9d4eeaf6f9d1272000aa598c8461afc330a8e65f263d45b0eab222f8ddfec71 guix-build-5474f5c356c5/output/powerpc64le-linux-gnu/bitcoin-5474f5c356c5-powerpc64le-linux-gnu.tar.gz
f83ee11c35001d34f9fba7d1e9e201a4cc6fa5e44bd51e13dabffcb35324348c guix-build-5474f5c356c5/output/riscv64-linux-gnu/SHA256SUMS.part
c441e56f23f224122ed064cdb57364fb129f5c9d50c5e8173952ce649b46bdb8 guix-build-5474f5c356c5/output/riscv64-linux-gnu/bitcoin-5474f5c356c5-riscv64-linux-gnu-debug.tar.gz
319db8af21a4d3c7bbdf54c315ad70bacf7fba1f2559408188d90c9ba60ca63c guix-build-5474f5c356c5/output/riscv64-linux-gnu/bitcoin-5474f5c356c5-riscv64-linux-gnu.tar.gz
9ac536a04d7e500f87b1f7dfb60e1e84cde2c192d3dffb89c308b864e9b9d583 guix-build-5474f5c356c5/output/x86_64-apple-darwin/SHA256SUMS.part
19e70f13fb4bf82375f7ca882a23e831f84729278e643cf5911182bdababa893 guix-build-5474f5c356c5/output/x86_64-apple-darwin/bitcoin-5474f5c356c5-x86_64-apple-darwin-unsigned.dmg
673a20e9457af3d0a89ea6721f84c6136132d3fbe469b7371bf14ce688b567d0 guix-build-5474f5c356c5/output/x86_64-apple-darwin/bitcoin-5474f5c356c5-x86_64-apple-darwin-unsigned.tar.gz
74162d53faffc4372ae4587cde395fe078b5c440c43c5a4ad8b8b890e9546255 guix-build-5474f5c356c5/output/x86_64-apple-darwin/bitcoin-5474f5c356c5-x86_64-apple-darwin.tar.gz
d751d50427d7abcbe9ac1daf087dc3addb7e4e6b90bb4c3ef6c31f8e54cac25e guix-build-5474f5c356c5/output/x86_64-linux-gnu/SHA256SUMS.part
49951b53172d4fe193d7ffc15b04a4bc058a3209653982b65912f2b221305dd4 guix-build-5474f5c356c5/output/x86_64-linux-gnu/bitcoin-5474f5c356c5-x86_64-linux-gnu-debug.tar.gz
93d4c4b07202a9171a72b6887e3931e53b3a8c22433b26521e2cb2a0c942f52a guix-build-5474f5c356c5/output/x86_64-linux-gnu/bitcoin-5474f5c356c5-x86_64-linux-gnu.tar.gz
b4b79e3578b6ffbb3075aacff969fb201b386e35a2d9d597047b61ff14bdfbfb guix-build-5474f5c356c5/output/x86_64-w64-mingw32/SHA256SUMS.part
8260f5c9a38cc577dff2143f00c465d117aa9835b3633365289d6807d7e46e7c guix-build-5474f5c356c5/output/x86_64-w64-mingw32/bitcoin-5474f5c356c5-win64-debug.zip
b20fbf02ddf617b86e5d5f88346ed97f9d169cd56904684ba3ca5f03ea85f008 guix-build-5474f5c356c5/output/x86_64-w64-mingw32/bitcoin-5474f5c356c5-win64-setup-unsigned.exe
16e9d1b817a832bfb0be2b9065440245a5d04b3aae8e34ff0f43f20c5dd7047f guix-build-5474f5c356c5/output/x86_64-w64-mingw32/bitcoin-5474f5c356c5-win64-unsigned.tar.gz
da766e257d10cf8890a530babc6100039c69ae7ed8e4f969eb612b4a411dd88f guix-build-5474f5c356c5/output/x86_64-w64-mingw32/bitcoin-5474f5c356c5-win64.zip
```
ACKs for top commit:
adam2k:
ACK 5474f5c356c5a17fbf6c84110cf83a5753cd0367
achow101:
ACK 5474f5c356c5a17fbf6c84110cf83a5753cd0367
jarolrod:
ACK 5474f5c356c5a17fbf6c84110cf83a5753cd0367
Tree-SHA512: d5595c96d0386b5ee9b98d9499770a00d8b751500020bf92f75c88e688640a50cfa5ebe7c26dea8cc5473b16a6adb83ec39891dd18d32ef59df5cf48d7091d6c
|
|
The same rule is used by the tests and benchmarks to generate headers,
and currently causes #25501. Just deduplicate the code into Makefile.am.
|
|
make clean currently looks for test.cpp.log, when it should be test.log.
|
|
Rename `BResult` class to `util::Result` and update the class interface to be
more compatible with `std::optional` and with a full-featured result class
implemented in https://github.com/bitcoin/bitcoin/pull/25665. Motivation for
this change is to update existing `BResult` usages now so they don't have to
change later when more features are added in #25665.
This change makes the following improvements originally implemented in #25665:
- More explicit API. Drops potentially misleading `BResult` constructor that
treats any bilingual string argument as an error. Adds `util::Error`
constructor so it is never ambiguous when a result is being assigned an error
or non-error value.
- Better type compatibility. Supports `util::Result<bilingual_str>` return
values to hold translated messages which are not errors.
- More standard and consistent API. `util::Result` supports most of the same
operators and methods as `std::optional`. `BResult` had a less familiar
interface with `HasRes`/`GetObj`/`ReleaseObj` methods. The Result/Res/Obj
naming was also not internally consistent.
- Better code organization. Puts `src/util/` code in the `util::` namespace so
naming reflects code organization and it is obvious where the class is coming
from. Drops "B" from name because it is undocumented what it stands for
(bilingual?)
- Has unit tests.
|
|
during coin selection
71d1d13627ccd27319f347e2d8167c8fe8a433f4 test: add unit test for AvailableCoins (josibake)
da03cb41a4ce15ebceee7fa4a4fdd2d3602fe284 test: functional test for new coin selection logic (josibake)
438e04845bf3302b7f459a50e88a1b772527f1e6 wallet: run coin selection by `OutputType` (josibake)
77b07072061c59f50c69be29fbcddf0d433e1077 refactor: use CoinsResult struct in SelectCoins (josibake)
2e67291ca3ab2d8f498fa910738ca655fde11c5e refactor: store by OutputType in CoinsResult (josibake)
Pull request description:
# Concept
Following https://github.com/bitcoin/bitcoin/pull/23789, Bitcoin Core wallet will now generate a change address that matches the payment address type. This improves privacy by not revealing which of the outputs is the change at the time of the transaction in scenarios where the input address types differ from the payment address type. However, information about the change can be leaked in a later transaction. This proposal attempts to address that concern.
## Leaking information in a later transaction
Consider the following scenario:
![mix input types(1)](https://user-images.githubusercontent.com/7444140/158597086-788339b0-c698-4b60-bd45-9ede4cd3a483.png)
1. Alice has a wallet with bech32 type UTXOs and pays Bob, who gives her a P2SH address
2. Alice's wallet generates a P2SH change output, preserving her privacy in `txid: a`
3. Alice then pays Carol, who gives her a bech32 address
4. Alice's wallet combines the P2SH UTXO with a bech32 UTXO and `txid: b` has two bech32 outputs
From a chain analysis perspective, it is reasonable to infer that the P2SH input in `txid: b` was the change from `txid: a`. To avoid leaking information in this scenario, Alice's wallet should avoid picking the P2SH output and instead fund the transaction with only bech32 Outputs. If the payment to Carol can be funded with just the P2SH output, it should be preferred over the bech32 outputs as this will convert the P2SH UTXO to bech32 UTXOs via the payment and change outputs of the new transaction.
**TLDR;** Avoid mixing output types, spend non-default `OutputTypes` when it is economical to do so.
# Approach
`AvailableCoins` now populates a struct, which makes it easier to access coins by `OutputType`. Coin selection tries to find a funding solution by each output type and chooses the most economical by waste metric. If a solution can't be found without mixing, coin selection runs over the entire wallet, allowing mixing, which is the same as the current behavior.
I've also added a functional test (`test/functional/wallet_avoid_mixing_output_types.py`) and unit test (`src/wallet/test/availablecoins_tests.cpp`.
ACKs for top commit:
achow101:
re-ACK 71d1d13627ccd27319f347e2d8167c8fe8a433f4
aureleoules:
ACK 71d1d13627ccd27319f347e2d8167c8fe8a433f4.
Xekyo:
reACK 71d1d13627ccd27319f347e2d8167c8fe8a433f4 via `git range-diff master 6530d19 71d1d13`
LarryRuane:
ACK 71d1d13627ccd27319f347e2d8167c8fe8a433f4
Tree-SHA512: 2e0716efdae5adf5479446fabc731ae81d595131d3b8bade98b64ba323d0e0c6d964a67f8c14c89c428998bda47993fa924f3cfca1529e2bd49eaa4e31b7e426
|
|
Test each component of the RBF policy in isolation. Unlike the RBF
functional tests, these do not rely on things like RPC results, mempool
submission, etc.
|
|
test that UTXOs are bucketed correctly after
running AvailableCoins
|
|
|
|
|
|
|
|
f3a50c9dfe645c548713e44e0eaf26ea9917a379 miniscript: rename IsSane and IsSaneSubexpression to prevent misuse (Antoine Poinsot)
c5fe5163dc31db939c44129f2ff8283b290a9330 miniscript: nit: don't return after assert(false) (Antoine Poinsot)
7bbaca9d8d355a17348a8d01e3e2521c5de466b0 miniscript: explicit the threshold size computation in multi() (Antoine Poinsot)
8323e4249db50d46ae4f43c1d8a50666549ae938 miniscript: add an OpCode typedef for readability (Antoine Poinsot)
7a549c6c59e6babbae76af008433426c6fa38fe2 miniscript: mark nodes with duplicate keys as insane (Antoine Poinsot)
8c0f8bf7bc3750fad648af1a548517a272114bca fuzz: add a Miniscript target for string representation roundtripping (Antoine Poinsot)
be34d5077b2fede7404de7706362f5858c443525 fuzz: rename and improve the Miniscript Script roundtrip target (Antoine Poinsot)
7eb70f0ac0a54adabc566e2b93bbf6b2beb54a79 miniscript: tiny doc fixups (Antoine Poinsot)
5cea85f12cba5dcfe3a298eddfa711f582adffac miniscript: split ValidSatisfactions from IsSane (Antoine Poinsot)
a0f064dc1474a048e236bfff12f4def3aa11daf3 miniscript: introduce a CheckTimeLocksMix helper (Antoine Poinsot)
ed45ee3882e69266d550b56ff69388e071f0ad1b miniscript: use optional instead of bool/outarg (Antoine Poinsot)
1ab8d89fd1bdb3c0f2a506b4a10df6c23ba21c48 miniscript: make equality operator non-recursive (Antoine Poinsot)
5922c662c08a061b3b3d5ac34a31f9f9d4640d47 scripted-diff: miniscript: rename 'nodetype' variables to 'fragment' (Antoine Poinsot)
c5f65db0f03b52bc4525acae944173829290ce6f miniscript: remove a workaround for a GCC 4.8 bug (Antoine Poinsot)
Pull request description:
The Miniscript repository and the Miniscript integration PR here have been a moving target for the past months, and some final cleanups were done there that were not included here. I initially intended to add some small followup commits to #24148 but i think there are enough of them to be worth a followup PR on its own.
Some parts of the code did not change since it was initially written in 2019, and the code could use some modernization. (Use std::optional instead of out args, remove old compiler workarounds).
We refactored the helpers to be more meaningful, and also did some renaming. A new fuzz target was also added and both were merged in a single file. 2 more will be added in #24149 that will be contained in this file too.
The only behaviour change in this PR is to rule out Miniscript with duplicate keys from sane Miniscripts. In a P2WSH context, signatures can be rebounded (Miniscript does not use CODESEPARATOR) and it's reasonable to assume that reusing keys across the Script drops the malleability guarantees.
It was previously assumed such Miniscript would never exist in the first place since a compiler should never create them. We finally agreed that if one were to exist (say, written by hand or from a buggy compiler) it would be very confusing if an imported Miniscript descriptor (after #24148) with duplicate keys was deemed sane (ie, "safe to use") by Bitcoin Core. We now check for duplicate keys in the constructor.
This is (still) joint work with Pieter Wuille. (Actually he entirely authored the cleanups and code modernization.)
ACKs for top commit:
sipa:
utACK f3a50c9dfe645c548713e44e0eaf26ea9917a379 (with the caveat that a lot of it is my own code)
sanket1729:
code review ACK f3a50c9dfe645c548713e44e0eaf26ea9917a379. Did not review the fuzz tests.
Tree-SHA512: c043325e4936fe25e8ece4266b46119e000c6745f88cea530fed1edf01c80f03ee6f9edc83b6e9d42ca01688d184bad16bfd967c5bb8037744e726993adf3deb
|
|
Move bdb cppflags out of the catch-all BITCOIN_INCLUDES, and pass them
only where they are needed, which is in libbitcoin_node/wallet and the
tests.
|
|
The benchmarks are run as part of `make check` for a minimum sanity
check. The actual results are being ignored. So only run them for one
iteration.
This makes the `bench_bitcoin` part take 2m00 instead of 5m20 here.
Which is still too long (imo), but this needs to be solved in the
`WalletLoading*` benchmarks which take that long per iteration.
|
|
This fixes a blk file size calculation made during reindex that results in increased blk file malformity.
The fix is to avoid double counting the size of the serialization header during reindex.
This adds a unit test to reproduce the bug before the fix and to ensure that it does not recur.
These changes include a log message change also so as to not be as alarming. This is a common and recoverable
data corruption. These messages can now be filtered by the debug log reindex category.
|
|
Parse also key hashes using the Key type. Make this target the first of
the 4 Miniscript fuzz targets in a single `miniscript` file.
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
|
|
035fa1f07aacb7bce74c0884ae28c8cf00fe3b1b build: Remove LIBTOOL_APP_LDFLAGS for bitcoin-chainstate (Cory Fields)
3f0595095dd6d230dc661641227937e3ab4ca8d3 docs: Add libbitcoinkernel_la_SOURCES explanation (Carl Dong)
94ad45deb257a95b4e98aa85da0371fb072fcd4c ci: Build libbitcoinkernel (Carl Dong)
26b2e7ffb3471a4712e5b9e50e066e0e3218f0dd build: Extract the libbitcoinkernel library (Carl Dong)
1df44dd20ca9e6e55eb353824b27d11bd1878c59 b-cs: Define G_TRANSLATION_FUN in bitcoinkernel.cpp (Carl Dong)
83a0bb7cc9907dbe089409ed5a417277ed63ed95 build: Separate lib_LTLIBRARIES initialization (Carl Dong)
c1e16cb31f4d8edde8fea310011189b8b272cb07 build: Create .la library for bitcoincrypto (Carl Dong)
8bdfe057c796dde1cd2e5a37a73e87a879e9fe56 build: Create .la library for leveldb (Carl Dong)
05d1525b6d4412f68ff4c5460cd1daa6fb49969b build: Create .la library for crc32c (Carl Dong)
64caf944797bc35c3044fe5675389656f9511a41 build: Remove vestigial LIBLEVELDB_SSE42 (Carl Dong)
1392e8e2d8cfe4115f0a152aca16ffe3f0f4573a build: Don't add unrelated libs to LIBTEST_* (Carl Dong)
Pull request description:
Part of: #24303
This PR introduces a `libbitcoinkernel` static library linking in the minimal list of files necessary to use our consensus engine as-is. `bitcoin-chainstate` introduced in #24304 now will link against `libbitcoinkernel`.
Most of the changes are related to the build system.
Please read the commit messages for more details.
ACKs for top commit:
theuni:
This may be my favorite PR ever. It's a privilege to ACK 035fa1f07aacb7bce74c0884ae28c8cf00fe3b1b.
Tree-SHA512: b755edc3471c7c1098847e9b16ab182a6abb7582563d9da516de376a770ac7543c6fdb24238ddd4d3d2d458f905a0c0614b8667aab182aa7e6b80c1cca7090bc
|
|
|
|
- LIBLEVELDB_SSE42_INT was defined, but never referenced anywhere
- LIBLEVELDB_SSE42 is referenced, but never defined anywhere
Apparently leveldb used to have platform-specific crc32 code before it
got split off into a separate lib.
|
|
loading
54b39cfb342d10a448d49299c715e3a25c2aca4a Add release notes (stickies-v)
f959fc0397c3f3615e99bc28d2df549d9d52f277 Update /<count>/ endpoints to use a '?count=' query parameter instead (stickies-v)
a09497614e9bb603fff36286d9611a25b23eeb02 Add GetQueryParameter helper function (stickies-v)
fff771ee864975cee8c831651239bac95503c37a Handle query string when parsing data format (stickies-v)
c1aad1b3b95b7c6bdf05e0c2095aba2f2db8310b scripted-diff: rename RetFormat to RESTResponseFormat (stickies-v)
9f1c54787c81177dd56a31c881a9ad2834a122dc Refactoring: move declarations to rest.h (stickies-v)
Pull request description:
In RESTful APIs, [typically](https://rapidapi.com/blog/api-glossary/parameters/query/) path parameters (e.g. `/some/unique/resource/`) are used to represent resources, and query parameters (e.g. `?sort=asc`) are used to control how these resources are being loaded through e.g. sorting, pagination, filtering, ...
As first [discussed in #17631](https://github.com/bitcoin/bitcoin/pull/17631#discussion_r733031180), the [current REST api](https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md) contains two endpoints `/headers/` and `/blockfilterheaders/` that rather unexpectedly use path parameters to control how many (filter) headers are returned in the response. While this is no critical issue, it is unintuitive and we are still early enough to easily phase this behaviour out and ensure new endpoints (if any) do not have to stick to non-standard behaviour just for internal consistency.
In this PR, a new `HTTPRequest::GetQueryParameter` method is introduced to easily parse query parameters, as well as two new `/headers/` and `/blockfilterheaders/` endpoints that use a count query parameter are introduced. The old path parameter-based endpoints are kept without too much overhead, but the documentation now points to the new query parameter-based endpoints as the default interface to encourage standardness.
## Behaviour change
### New endpoints and default values
`/headers/` and `/blockfilterheaders/` now have 2 new endpoints that contain query parameters (`?count=<count>`) instead of path parameters (`/<count>/`), as described in REST-interface.md. Since query parameters can easily have default values, I have set this at 5 for both endpoints.
**headers**
`GET /rest/headers/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>`
should now be used instead of
`GET /rest/headers/<COUNT>/<BLOCK-HASH>.<bin|hex|json>`
**blockfilterheaders**
`GET /rest/blockfilterheaders/<FILTERTYPE>/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5>`
should now be used instead of
`GET /rest/blockfilterheaders/<FILTERTYPE>/<COUNT>/<BLOCK-HASH>.<bin|hex|json>`
### Some previously invalid API calls are now valid
API calls that contained query strings in the URI could not be parsed prior to this PR. This PR changes behaviour in that previously invalid calls (e.g. `GET /rest/headers/5/somehash.json?someunusedparam=foo`) would now become valid, as the query parameters are properly parsed, and discarded if unused.
For example, prior to this PR, adding an irrelevant `someparam` parameter would be illegal:
```
GET /rest/headers/5/0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?someparam=true
->
Invalid hash: 0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?someparam=true
```
**This behaviour change affects all rest endpoints, not just the 2 new ones introduced here.**
*(Note: I'd be open to implementing additional logic to refuse requests containing unrecognized query parameters to minimize behaviour change, but for the endpoints that we currently have I don't really see the point for that added complexity. E.g. I don't see any scenarios where misspelling a parameter could lead to harmful outcomes)*
## Using the REST API
To run the API HTTP server, start a bitcoind instance with the `-rest` flag enabled. To use the
`blockfilterheaders` endpoint, you'll also need to set `-blockfilterindex=1`:
```
./bitcoind -signet -rest -blockfilterindex=1
```
As soon as bitcoind is fully up and running, you should be able to query the API, for example by
using curl on the command line: ```curl "127.0.0.1:38332/rest/chaininfo.json"```.
To more easily parse the JSON output, you can also use tools like 'jq' or `json_pp`, e.g.:
```
curl -s "localhost:38332/rest/blockfilterheaders/basic/0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?count=2" | json_pp .
```
## To do
- [x] update `doc/release-notes`
## Feedback
This is my first PR (hooray!). Please don't hold back on any feedback/comments/nits/... you may have, big or small, whether they are code, process, language, ... related. I welcome private messages too if there's anything you don't want to clutter the PR with. I'm here to learn and am grateful for everyone's input.
ACKs for top commit:
stickies-v:
I've had to push a tiny doc update to `REST-interface.md` (`git range-diff 219d728 9aac438 54b39cf`) since this was not merged for v23, but since there are no significant changes beyond theStack and jnewbery's ACKs I think this PR is now ready to be considered for merging? @MarcoFalke
jnewbery:
ACK 54b39cfb342d10a448d49299c715e3a25c2aca4a
theStack:
re-ACK 54b39cfb342d10a448d49299c715e3a25c2aca4a
Tree-SHA512: 3b393ffde34f25605ca12c0b1300799a19684b816a1d03aed38b0f5439df47bfe6a589ffbcd7b83fd2def6c9d00a1bae5e45b1d18df4ae998c617c709990f83f
|
|
2da94a4c6f55f7a3621f4a6f70902c52f735c868 fuzz: add a fuzz target for Miniscript decoding from Script (Antoine Poinsot)
f8369996e76dbc41a12f7b7eea14a7e7990a81c1 Miniscript: ops limit and stack size computation (Pieter Wuille)
2e55e88f86d0dd49b35d04af3f57e863498aabae Miniscript: conversion from script (Pieter Wuille)
1ddaa66eae67b102f5e37d212d366a5dcad4aa26 Miniscript: type system, script creation, text notation, tests (Pieter Wuille)
4fe29368c0ded0e62f437cab3a7c904f7fd3ad67 script: expose getter for CScriptNum, add a BuildScript helper (Antoine Poinsot)
f4e289f384efdda6c3f56e1e1c30820a91ac2612 script: move CheckMinimalPush from interpreter to script.h (Antoine Poinsot)
31ec6ae92a5d9910a26d90a6ff20bab27dee5826 script: make IsPushdataOp non-static (Antoine Poinsot)
Pull request description:
Miniscript is a language for writing (a subset of) Bitcoin Scripts in a structured way.
Miniscript permits:
- To safely extend the Output Descriptor language to many more scripting features thanks to the typing system (composition).
- Statical analysis of spending conditions, maximum spending cost of each branch, security properties, third-party malleability.
- General satisfaction of any correctly typed ("valid" [0]) Miniscript. The satisfaction itself is also analyzable.
- To extend the possibilities of external signers, because of all of the above and since it carries enough metadata.
Miniscript guarantees:
- That for any statically-analyzed as "safe" [0] Script, a witness can be constructed in the bounds of the consensus and standardness rules (standardness complete).
- That unless the conditions of the Miniscript are met, no witness can be created for the Script (consensus sound).
- Third-party malleability protection for the satisfaction of a sane Miniscript, which is too complex to summarize here.
For more details around Miniscript (including the specifications), please refer to the [website](https://bitcoin.sipa.be/miniscript/).
Miniscript was designed by Pieter Wuille, Andrew Poelstra and Sanket Kanjalkar.
This PR is an updated and rebased version of #16800. See [the commit history of the Miniscript repository](https://github.com/sipa/miniscript/commits/master) for details about the changes made since September 2019 (TL;DR: bugfixes, introduction of timelock conflicts in the type system, `pk()` and `pkh()` aliases, `thresh_m` renamed to `multi`, all recursive algorithms were made non-recursive).
This PR is also the first in a series of 3:
- The first one (here) integrates the backbone of Miniscript.
- The second one (#24148) introduces support for Miniscript in Output Descriptors, allowing for watch-only support of Miniscript Descriptors in the wallet.
- The third one (#24149) implements signing for these Miniscript Descriptors, using Miniscript's satisfaction algorithm.
Note to reviewers:
- Miniscript is currently defined only for P2WSH. No Taproot yet.
- Miniscript is different from the policy language (a high-level logical representation of a spending policy). A policy->Miniscript compiler is not included here.
- The fuzz target included here is more interestingly extended in the 3rd PR to check a script's satisfaction against `VerifyScript`. I think it could be further improved by having custom mutators as we now have for multisig (see https://github.com/bitcoin/bitcoin/issues/23105). A minified corpus of Miniscript Scripts is available at https://github.com/bitcoin-core/qa-assets/pull/85.
[0] We call "valid" any correctly-typed Miniscript. And "safe" any sane Miniscript, ie one whose satisfaction isn't malleable, which requires a key for any spending path, etc..
ACKs for top commit:
jb55:
ACK 2da94a4c6f55f7a3621f4a6f70902c52f735c868
laanwj:
Light code review ACK 2da94a4c6f55f7a3621f4a6f70902c52f735c868 (mostly reviewed the changes to the existing code and build system)
Tree-SHA512: d3ef558436cfcc699a50ad13caf1e776f7d0addddb433ee28ef38f66ea5c3e581382d8c748ccac9b51768e4b95712ed7a6112b0e3281a6551e0f325331de9167
|
|
21520b95515676d45145df624f430cdd39db7515 fuzz: add target for coinselection (Martin Zumsande)
Pull request description:
This adds a fuzz target for the coinselection algorithms by creating random `OutputGroup`s and running all three coin selection algorithms for them.
It does not fuzz higher-level wallet logic for selecting eligible coins (as in `SelectCoins()`), thought it probably would make sense to have a fuzz target for that too.
ACKs for top commit:
achow101:
ACK 21520b95515676d45145df624f430cdd39db7515
vasild:
ACK 21520b95515676d45145df624f430cdd39db7515
Tree-SHA512: c763003cf5ff5317f929d3d0b2f06fa739ae41dd642042d9a5c5c96e6cb9b349a6c7aeabc77bc2b846d12c8bcb60e07ee20a9f38539429c65723ab76aeee6b2e
|
|
This creates random OutputGroups and runs the
existing coinselection algorithms for them.
|
|
|
|
Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
|
|
f8cba0d9117fe9b9ac51d7044372b28270c7838b test: Change default test logging directory (Yancy Ribbens)
Pull request description:
This PR changes the default test log location request here: https://github.com/bitcoin/bitcoin/issues/17224. Instead of using the location of the makefile [automatic variable](https://www.gnu.org/software/make/manual/make.html#Automatic-Variables) `$<` I extract just the basename and then prepend a new location `./test`. This is done because `$<` represents the variable name AND location of the prerequisite here.
Top commit has no ACKs.
Tree-SHA512: f0fbc530cf0e14c284b4bbf6671c145b1d7a2e1f5561f5c5d09f0cbe88b98e620e763bbbf2dfa9aeeec3dcc9b0127939e105e14c7e4f6660c7c19663622a393d
|