92993aa5cf Change SignTransaction's input_errors to use bilingual_str (Andrew Chow)
171366e89b Use bilingual_str for address fetching functions (Andrew Chow)
9571c69b51 Add bilingual_str::clear() (Andrew Chow)
Pull request description:
In a couple of places in the wallet, errors are `std::string`. In order for these errors to be translated, change them to use `bilingual_str`.
ACKs for top commit:
hebasto:
re-ACK 92993aa5cf, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22337#pullrequestreview-694542729) review, verified with
klementtan:
Code review ACK 92993aa5cf
meshcollider:
Code review ACK 92993aa5cf
Tree-SHA512: 5400e419dd87db8c49b67ed0964de2d44b58010a566ca246f2f0760ed9ef6a9b6f6df7a6adcb211b315b74c727bfe8c7d07eb5690b5922fa5828ceef4c83461f
87651795d8 fuzz: check that ser+unser produces the same AddrMan (Vasil Dimov)
6408b24517 fuzz: move init code to the CAddrManDeterministic constructor (Vasil Dimov)
Pull request description:
Add a fuzz test that fills addrman with a pile of randomly generated addresses, serializes it to a stream, unserializes the stream to another addrman object and compares the two.
Some discussion of this already happened at https://github.com/jnewbery/bitcoin/pull/18.
ACKs for top commit:
practicalswift:
cr ACK 87651795d8
jonatack:
ACK 87651795d8 rebased to current master, reviewed, fuzz build, ran `FUZZ=addrman_serdeser src/test/fuzz/fuzz`
Tree-SHA512: 7eda79279f14f2649840bf752e575d7b02cbaad541f74f7254855ebd4a32da988f042d78aa9228983350283bb74dd0c71f51f04c0846889c3ba2f19f01a0c303
fa1eddb1a3 Fix whitespace in touched files (MarcoFalke)
fa4e6afdae Remove unused CSubNet serialize code (MarcoFalke)
fa384fdd0b Ignore banlist.dat (MarcoFalke)
Pull request description:
The code to read `banlist.dat` should be removed eventually. The major release (22.x) can be used to translate a `banlist.dat` into a `banlist.json`. Thus, it is now possible to remove the reading code.
ACKs for top commit:
Zero-1729:
re-ACK fa1eddb1a3
laanwj:
concept and code review ACK fa1eddb1a3
vasild:
ACK fa1eddb1a3
jonatack:
Light code review utACK fa1eddb1a3
Tree-SHA512: e136193b7c0ba1d6d2e79c7fb4106ba4af75fa229ed7214675ee64e98e59bb4808779e7a8a09eecce62f7a5d4bc6e16b8a5ad4596129357c8fc5e3b88f214249
fae108ceb5 Fix incorrect whitespace in addrman (MarcoFalke)
fa32024d51 Add missing GUARDED_BY to CAddrMan::insecure_rand (MarcoFalke)
fab755b77f fuzz: Actually use const addrman (MarcoFalke)
fae0c79351 refactor: Mark CAddrMan::GetAddr const (MarcoFalke)
fa02934c8c refactor: Mark CAddrMan::Select const (MarcoFalke)
Pull request description:
To clarify that a call to this only changes the random state and nothing else.
ACKs for top commit:
jnewbery:
Code review ACK fae108ceb5
theStack:
re-ACK fae108ceb5🍦
Tree-SHA512: 3ffb211d4715cc3daeb3bfcdb3fcc6b108ca96df5fa565510436fac0e8da86c93b30c9c4aad0563e27d84f615fcd729481072009a4e2360c8b3d40787ab6506a
Leaving the incorrect indentation would be frustrating because:
* Some editor may fix up the whitespace when editing a file, so before
commiting the whitespace changes need to be undone.
* It makes it harder to use clang-format-diff on a change.
Can be trivially reviewed with --word-diff-regex=. --ignore-all-space
e4c8bb62e4 build: Fix undefined reference to __mulodi4 (Hennadii Stepanov)
Pull request description:
When compiling with clang on 32-bit systems the `__mulodi4` symbol is defined in compiler-rt only.
Fixes #21294.
See more:
- https://bugs.llvm.org/show_bug.cgi?id=16404
- https://bugs.llvm.org/show_bug.cgi?id=28629
ACKs for top commit:
MarcoFalke:
tested-only ACK e4c8bb62e4
luke-jr:
utACK e4c8bb62e4
fanquake:
ACK e4c8bb62e4 - it's a bit of an awkward workaround to carry, but at-least it's contained to the fuzzers.
Tree-SHA512: 93edb4ed568027702b1b9aba953ad50889b834ef97fde3cb99d1ce70076d9c00aa13f95c86b12d6f59b24fa90108d93742f920e15119901a2848fb337ab859a1
fa33ed4b3f fuzz: Limit max ops in tx_pool fuzz targets (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Reproducer from OSS-Fuzz (without bug report):
[clusterfuzz-testcase-tx_pool_standard-5963992253202432.log](https://github.com/bitcoin/bitcoin/files/6822465/clusterfuzz-testcase-tx_pool_standard-5963992253202432.log)
ACKs for top commit:
practicalswift:
cr ACK fa33ed4b3f
Tree-SHA512: 32098d573880afba12d510ac83519dc886a6c65d5207edb810f92c7c61edf5e2fc9c57e7b7a1ae656c02ce14e3595707dd6b93caf7956beb2bc817609e14d23d
faa86b71ac fuzz: Use ConsumeUInt256 helper to simplify rolling_bloom_filter fuzz test (MarcoFalke)
aaaa61fd30 fuzz: Speed up rolling_bloom_filter fuzz test (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Reproducer from OSS-Fuzz (without bug report):
[clusterfuzz-testcase-rolling_bloom_filter-5980807721254912.log](https://github.com/bitcoin/bitcoin/files/6822159/clusterfuzz-testcase-rolling_bloom_filter-5980807721254912.log)
ACKs for top commit:
practicalswift:
cr ACK faa86b71ac
theStack:
Concept and code review ACK faa86b71ac
Tree-SHA512: eace588509dfddb2ba97baf86379fa713fa6eb758184abff676cb95807ff8ff36905eeaddeba05665b8464c35c57e2138f88caec71cbfb255e546bbe76558da0
faafda232e fuzz: Speed up prevector fuzz target (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=35981
ACKs for top commit:
practicalswift:
cr ACK faafda232e
Tree-SHA512: 1bf166c4a99a8ce88bdc030cd6a32ce1da5251b73873772e0e9c001ec2bacafebb183f7c8c88806d0ab633aada2cff8b78791f5c9c0c6f2cc8ef5f0875c4b2ef
fa8bed6a47 fuzz: Temporarily disable failing assert in banman fuzz test (MarcoFalke)
Pull request description:
Otherwise the remainder of the fuzz test can't be fuzzed without running into crashes
ACKs for top commit:
practicalswift:
cr ACK fa8bed6a47
Tree-SHA512: ec6606292e2cfd26484c7f6caf1c418c377da54111b332990fce68373f0438defda71d931a42ca34431527fbc172dd2fdf29b260afca15b34910ee137de1c365
aaaa9c6019 fuzz: Extend addrman fuzz test with deserialize (MarcoFalke)
Pull request description:
Requested on IRC:
```
[18:01] <vasild> => I think there is a good chance fuzzing addrman unserialize will find more bugs
[18:04] <sipa> definitely
ACKs for top commit:
jonatack:
ACK aaaa9c6019 per `git diff fa74025 aaaa9c6`
vasild:
ACK aaaa9c6019
Tree-SHA512: f57d0aecf22a933e48d3181d7398218949588dd0de31218d1d28c825649e55fd60b0de6fbc92d2497cf5639a4adc2061c9bf8216546a2be916feac4f03f16e8f
f036dfbb69 [addrman] Remove unused test_before_evict argument from Good() (John Newbery)
Pull request description:
This has never been used in the public interface method since it was
introduced in #9037.
ACKs for top commit:
lsilva01:
Tested ACK f036dfbb69 on Ubuntu 20.04.
theStack:
Code-review ACK f036dfbb69
Tree-SHA512: 98145d9596b4ae1f354cfa561be1a54c6b8057c920e0ac3d4c1d42c9326b2dad2d44320f4171bb701d97088b216760cca8017b84c8b5dcd2b1dc8f158f28066d
fa621ededd refactor: Pass script verify flags as uint32_t (MarcoFalke)
Pull request description:
The flags are cast to unsigned in the interpreter anyway, so avoid the confusion (and fuzz crashes) by just passing them as unsigned from the beginning.
Also, the flags are often inverted bit-wise with the `~` operator, which also works on signed integers, but might cause confusion as the sign bit is flipped.
Fixes #22233
ACKs for top commit:
theStack:
Concept and code review ACK fa621ededd
kristapsk:
ACK fa621ededd
jonatack:
ACK fa621ededd
Tree-SHA512: ea0720f32f823fa7f075309978672aa39773c6019d12b6c1c9d611fc1983a76115b7fe2a28d50814673bb6415c311ccc05b99d6e871575fb6900faf75ed17769
facd56750c scripted-diff: Revert "fuzz: Add Temporary debug assert for oss-fuzz issue" (MarcoFalke)
Pull request description:
No longer needed, as it wouldn't help to debug this issue. See https://github.com/bitcoin/bitcoin/pull/22472#issuecomment-882692900
ACKs for top commit:
fanquake:
ACK facd56750c
Tree-SHA512: 13352b3529c43d6e65ab127134b32158d3072dc2fbbb326fea9adfeada5a8610d0477ea75748b8b68e7abb3b9869a989df3a3169e92bdd458053d64bae6ed379
Moved implementations of `ConsumeTxMemPoolEntry`, `ContainsSpentInput`, `ConsumeNetAddr`, and the methods(open, read, write, seek, close) of FuzzedFileProvider from test/fuzz/util.h to test/fuzz/util.cpp.
00b875ba94 addrman: remove invalid addresses when unserializing (Vasil Dimov)
bdb62096f0 fuzz: reduce possible networks check (Vasil Dimov)
a164cd3ba6 net: simplify CNetAddr::IsRoutable() (Vasil Dimov)
Pull request description:
* Simplify some code, now that we know `CNetAddr::IsRFC4193()` and `CNetAddr::IsTor()` cannot be `true` at the same time.
* Drop Tor v2 addresses when loading addrman from `peers.dat` - they would have been loaded as dummy-all-zeros IPv6 addresses and linger in addrman, wasting space.
ACKs for top commit:
sipa:
ACK 00b875ba94. Reviewed the code, and tested with -DDEBUG_ADDRMAN (unit tests + mainnet run with peers.dat that contained v2 onions).
laanwj:
Code review and lightly tested ACK 00b875ba94
jonatack:
ACK 00b875ba94 reviewed, debug-built with -DEBUG_ADDRMAN rebased to current master, restarted node on mainnet/signet/testnet and verified that on each chain -addrinfo shows no change in address counts (as expected). Added some sanity check asserts, rebuilt/re-ran test. Checked that the new test fails on master with "test/addrman_tests.cpp(824): error: in "addrman_tests/remove_invalid": check addrman.size() == 2 has failed [4 != 2]"
jarolrod:
ACK 00b875ba94
Tree-SHA512: 6ed8e6745134b1b94fffaba28482de909ea39483b46b7f57bda61cdbae7a51251d15cb674de3631772fbeabe153d77a19269f96e62a89102a2d5c01e48f0ba06
* Assert when a type is missing
* Add missing WitnessV1Taproot
* Limit WitnessUnknown to version [2, 16], to avoid abiguity
* Limit WitnessUnknown to size [2, 40], to avoid invalid sizes
fa0d9211ef refactor: Remove chainparams arg from CChainState member functions (MarcoFalke)
fa38947125 refactor: Remove ::Params() global from inside CChainState member functions (MarcoFalke)
Pull request description:
The `::Params()` global is verbose and confusing. Also it makes tests a bit harder to write because they'd have to mock a global.
Fix all issues by simply using a member variable that points to the right params.
(Can be reviewed with `--word-diff-regex=.`)
ACKs for top commit:
jnewbery:
ACK fa0d9211ef
kiminuo:
utACK fa0d9211
theStack:
ACK fa0d9211ef🍉
Tree-SHA512: 44676b19c9ed471ccb536331d3029bad192d7d50f394fd7b8527ec431452aeec8c4494164b9cf8e16e0123c4463b16be864366c6b599370032c17262625a0356
7ad414f4bf doc: add comment about CCoinsViewDBCursor constructor (James O'Beirne)
0f8a5a4dd5 move-only(ish): don't expose CCoinsViewDBCursor (James O'Beirne)
615c1adfb0 refactor: wrap CCoinsViewCursor in unique_ptr (James O'Beirne)
Pull request description:
I tripped over this one for a few hours at the beginning of the week, so I've sort of got a personal vendetta against `CCoinsView::Cursor()` returning a raw pointer.
Specifically in the case of CCoinsViewDB, if a raw cursor is allocated and not freed, a cryptic leveldb assertion failure occurs on CCoinsViewDB destruction (`Assertion 'dummy_versions_.next_ == &dummy_versions_' failed.`).
This is a pretty simple change.
Related to: https://github.com/bitcoin/bitcoin/issues/21766
See also: https://github.com/google/leveldb/issues/142#issuecomment-414418135
ACKs for top commit:
MarcoFalke:
review ACK 7ad414f4bf🔎
jonatack:
re-ACK 7ad414f4bf modulo suggestion
ryanofsky:
Code review ACK 7ad414f4bf. Two new commits look good and thanks for clarifying constructor comment
Tree-SHA512: 6471d03e2de674d84b1ea0d31e25f433d52aa1aa4996f7b4aab1bd02b6bc340b15e64cc8ea07bbefefa3b5da35384ca5400cc230434e787c30931b8574c672f9
Save the banlist in `banlist.json` instead of `banlist.dat`.
This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).
Only read `banlist.dat` if it exists and `banlist.json` does not
exist (first start after an upgrade).
Supersedes https://github.com/bitcoin/bitcoin/pull/20904
Resolves https://github.com/bitcoin/bitcoin/issues/19748
9550dffa0c fuzz: Assert roundtrip equality for `CPubKey` (Sebastian Falbesoner)
Pull request description:
This PR is a (quite late) follow-up to #19237 (https://github.com/bitcoin/bitcoin/pull/19237#issuecomment-642203251). Looking at `CPubKey::Serialize` and `CPubKey::Unserialize` I can't think of a scenario where the roundtrip (serialization/deserialization) equality wouldn't hold.
ACKs for top commit:
jamesob:
crACK 9550dffa0c pending CI
Tree-SHA512: 640fb9e777d249769b22ee52c0b15a68ff0645b16c986e1c0bce9742155d14f1be601e591833e1dc8dcffebf271966c6b861b90888a44aae1feae2e0248e2c55
f8866e8c32 Add roundtrip fuzz tests for CAddress serialization (Pieter Wuille)
e2f0548b52 Use addrv2 serialization in anchors.dat (Pieter Wuille)
8cd8f37dfe Introduce well-defined CAddress disk serialization (Pieter Wuille)
Pull request description:
Alternative to #20509.
This makes the `CAddress` disk serialization format well defined, and uses it to enable addrv2 support in anchors.dat (in a way that's compatible with older software). The new format is:
- The first 4 bytes store a format version number. Its low 19 bits are ignored (as those historically stored the `CLIENT_VERSION`), but its high 13 bits specify the actual serialization:
- 0x00000000: LE64 encoding for `nServices`, V1 encoding for `CService` (like pre-BIP155 network serialization).
- 0x20000000: CompactSize encoding for `nServices`, V2 encoding for `CService` (like BIP155 network serialization).
- Any other value triggers an unsupported format error on deserialization, and can be used for future format changes.
- The `ADDRV2_FORMAT` flag in the stream's version does not determine the actual serialization format; it only sets whether or not V2 encoding is permitted.
ACKs for top commit:
achow101:
ACK f8866e8c32
laanwj:
Code review ACK f8866e8c32
vasild:
ACK f8866e8c32
jonatack:
ACK f8866e8c32 tested rebased to master and built/run/restarted with DEBUG_ADDRMAN, peers.dat and anchors ser/deser seems fine
hebasto:
ACK f8866e8c32, tested on Linux Mint 20.1 (x86_64).
Tree-SHA512: 3898f8a8c51783a46dd0aae03fa10060521f5dd6e79315fe95ba807689e78f202388ffa28c40bf156c6f7b1fc2ce806b155dcbe56027df73d039a55331723796
