77c507358b Make Hash[160] consume range-like objects (Pieter Wuille)
02c4cc5c5d Make CHash256/CHash160 output to Span (Pieter Wuille)
0ef97b1b10 Make MurmurHash3 consume Spans (Pieter Wuille)
e549bf8a9a Make CHash256 and CHash160 consume Spans (Pieter Wuille)
2a2182c387 Make script/standard's BaseHash Span-convertible (Pieter Wuille)
e63dcc3a67 Add MakeUCharSpan, to help constructing Span<[const] unsigned char> (Pieter Wuille)
567825049f Make uint256 Span-convertible by adding ::data() (Pieter Wuille)
131a2f0337 scripted-diff: rename base_blob::data to m_data (Pieter Wuille)
Pull request description:
This makes use of the implicit constructions and conversions to Span introduced in #18468 to simplify the hash.h interface:
* All functions that take a pointer and a length are changed to take a Span instead.
* The Hash() and Hash160() functions are changed to take in "range" objects instead of begin/end iterators.
ACKs for top commit:
laanwj:
re-ACK 77c507358b
jonatack:
Code review re-ACK 77c5073 per `git range-diff 14ceddd 49fc016 77c5073`
Tree-SHA512: 9ec929891b1ddcf30eb14b946ee1bf142eca1442b9de0067ad6a3c181e0c7ea0c99c0e291e7f6e7a18bd7bdf78fe94ee3d5de66e167401674caf91e026269771
3bd67ba5a4 Test addr response caching (Gleb Naumenko)
cf1569e074 Add addr permission flag enabling non-cached addr sharing (Gleb Naumenko)
acd6135b43 Cache responses to addr requests (Gleb Naumenko)
7cc0e8101f Remove useless 2500 limit on AddrMan queries (Gleb Naumenko)
ded742bc5b Move filtering banned addrs inside GetAddresses() (Gleb Naumenko)
Pull request description:
This is a very simple code change with a big p2p privacy benefit.
It’s currently trivial to scrape any reachable node’s AddrMan (a database of all nodes known to them along with the timestamps).
We do have a limit of one GETADDR per connection, but a spy can disconnect and reconnect even from the same IP, and send GETADDR again and again.
Since we respond with 1,000 random records at most, depending on the AddrMan size it takes probably up to 100 requests for an spy to make sure they scraped (almost) everything.
I even have a script for that. It is totally doable within couple minutes.
Then, with some extra protocol knowledge a spy can infer the direct peers of the victim, and other topological stuff.
I suggest to cache responses to GETADDR on a daily basis, so that an attacker gets at most 1,000 records per day, and can’t track the changes in real time. I will be following up with more improvements to addr relay privacy, but this one alone is a very effective. And simple!
I doubt any of the real software does *reconnect to get new addrs from a given peer*, so we shouldn’t be cutting anyone.
I also believe it doesn’t have any negative implications on the overall topology quality. And the records being “outdated” for at most a day doesn’t break any honest assumptions either.
ACKs for top commit:
jnewbery:
reACK 3bd67ba5a4
promag:
Code review ACK 3bd67ba5a4.
ariard:
Code Review ACK 3bd67ba
Tree-SHA512: dfa5d03205c2424e40a3f8a41af9306227e1ca18beead3b3dda44aa2a082175bb1c6d929dbc7ea8e48e01aed0d50f0d54491caa1147471a2b72a46c3ca06b66f
fae8c28dae Pass mempool pointer to GetCoinsCacheSizeState (MarcoFalke)
fac674db20 Pass mempool pointer to UnloadBlockIndex (MarcoFalke)
faec851b6e test: Simplify cs_main locks (MarcoFalke)
Pull request description:
Split out from #19556
Instead of relying on the implicit mempool global, pass a mempool pointer (which can be `0`). This helps with testing, code clarity and unlocks the features described in #19556.
ACKs for top commit:
jnewbery:
code review ACK fae8c28dae
fjahr:
Code review ACK fae8c28dae
darosior:
Tested ACK fae8c28dae
jamesob:
ACK fae8c28dae ([`jamesob/ackr/19604.1.MarcoFalke.pass_mempool_pointer_to`](https://github.com/jamesob/bitcoin/tree/ackr/19604.1.MarcoFalke.pass_mempool_pointer_to))
Tree-SHA512: fa687518c8cda4a095bdbdfe56e01fae2fb16c13d51efbb1312cd6dc007611fc47f53f475602e4a843e3973c9410e6af5a81d6847bd2399f8262ca7205975728
bcfebb6d55 net: save the network type explicitly in CNetAddr (Vasil Dimov)
100c64a95b net: document `enum Network` (Vasil Dimov)
Pull request description:
(chopped off from https://github.com/bitcoin/bitcoin/pull/19031 to ease review)
Before this change, we would analyze the contents of `CNetAddr::ip[16]`
in order to tell which type is an address. Change this by introducing a
new member `CNetAddr::m_net` that explicitly tells the type of the
address.
This is necessary because in BIP155 we will not be able to tell the
address type by just looking at its raw representation (e.g. both TORv3
and I2P are "seemingly random" 32 bytes).
As a side effect of this change we no longer need to store IPv4
addresses encoded as IPv6 addresses - we can store them in proper 4
bytes (will be done in a separate commit). Also the code gets
somewhat simplified - instead of
`memcmp(ip, pchIPv4, sizeof(pchIPv4)) == 0` we can use
`m_net == NET_IPV4`.
ACKs for top commit:
troygiorshev:
reACK bcfebb6d55 via `git range-diff master 64897c5 bcfebb6`
jonatack:
re-ACK bcfebb6 per `git diff 662bb25 bcfebb6`, code review, debug build/tests clean, ran bitcoind.
laanwj:
Code review ACK bcfebb6d55
Tree-SHA512: 9347e2a50feac617a994bfb46a8f77e31c236bde882e4fd4f03eea4766cd5110216f5f3d24dee91d25218bab7f8bb6e1d2d6212a44db9e34594299fd6ff7606b
f19fdd47a6 test: add test for CChainState::ResizeCoinsCaches() (James O'Beirne)
8ac3ef4699 add ChainstateManager::MaybeRebalanceCaches() (James O'Beirne)
f36aaa6392 Add CChainState::ResizeCoinsCaches (James O'Beirne)
b223111da2 txdb: add CCoinsViewDB::ChangeCacheSize (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
In the assumeutxo implementation draft (#15056), once a UTXO snapshot is loaded, a new chainstate object is created after initialization. This means that we have to reclaim some of the cache that we've allocated to the original chainstate (per `dbcache=`) to repurpose for the snapshot chainstate.
Furthermore, it makes sense to have different cache allocations depending on which chainstate is more active. While the snapshot chainstate is working to get to the network tip (and the background validation chainstate is idle), it makes sense that the snapshot chainstate should have the majority of cache allocation. And contrariwise once the snapshot has reached network tip, most of the cache should be given to the background validation chainstate.
This set of changes (detailed in the commit messages) allows us to dynamically resize the various coins caches. None of the functionality introduced here is used at the moment, but will be in the next AU PR (which introduces `ActivateSnapshot`).
`ChainstateManager::MaybeRebalanceCaches()` defines the (somewhat normative) cache allocations between the snapshot and background validation chainstates. I'd be interested in feedback if anyone has thoughts on the proportions I've set there.
ACKs for top commit:
ajtowns:
weak utACK f19fdd47a6 -- didn't find any major problems, but not super confident that I didn't miss anything
fjahr:
Code review ACK f19fdd4
ryanofsky:
Code review ACK f19fdd47a6. Only change since last review is constructor cleanup (no change in behavior). I think the suggestions here from ajtowns and others are good, but shouldn't delay merging the PR (and hold up assumeutxo)
Tree-SHA512: fffb7847fb6993dd4a1a41cf11179b211b0b20b7eb5f7cf6266442136bfe9d43b830bbefcafd475bfd4af273f5573500594aa41fff03e0ed5c2a1e8562ff9269
Before this change, we would analyze the contents of `CNetAddr::ip[16]`
in order to tell which type is an address. Change this by introducing a
new member `CNetAddr::m_net` that explicitly tells the type of the
address.
This is necessary because in BIP155 we will not be able to tell the
address type by just looking at its raw representation (e.g. both TORv3
and I2P are "seemingly random" 32 bytes).
As a side effect of this change we no longer need to store IPv4
addresses encoded as IPv6 addresses - we can store them in proper 4
bytes (will be done in a separate commit). Also the code gets
somewhat simplified - instead of
`memcmp(ip, pchIPv4, sizeof(pchIPv4)) == 0` we can use
`m_net == NET_IPV4`.
Co-authored-by: Carl Dong <contact@carldong.me>
9c69cfe4c5 Add <datadir>/settings.json persistent settings storage. (Russell Yanofsky)
eb682c5700 util: Add ReadSettings and WriteSettings functions (Russell Yanofsky)
Pull request description:
Persistent settings are used in followup PRs #15936 to unify gui settings between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt.
ACKs for top commit:
MarcoFalke:
Approach re-ACK 9c69cfe4c5🌾
jnewbery:
utACK 9c69cfe4c5
Tree-SHA512: 39fcc6051717117c9141e934de1d0d3f739484be4685cdf97d54de967c8c816502b4fd0de12114433beaa5c5b7060c810fd8ae4e2b3ce7c371eb729ac01ba2e1
c8992e8959 test: Fix fuzzer compilation on macOS fixes#19557 (freenancial)
Pull request description:
fixes#19557
Before the fix:
```
➜ bitcoin git:(fix-fuzzer-macos) make
Making all in src
CXX test/fuzz/addition_overflow-addition_overflow.o
In file included from test/fuzz/addition_overflow.cpp:7:
./test/fuzz/util.h:335:13: error: no matching function for call to 'AdditionOverflow'
if (AdditionOverflow((uint64_t)fuzzed_file->m_offset, random_bytes.size())) {
^~~~~~~~~~~~~~~~
./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('unsigned long long' vs. 'unsigned long')
NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
^
./test/fuzz/util.h:346:13: error: no matching function for call to 'AdditionOverflow'
if (AdditionOverflow(fuzzed_file->m_offset, n)) {
^~~~~~~~~~~~~~~~
./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('long long' vs. 'long')
NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
^
```
After the fix:
```
➜ bitcoin git:(fix-fuzzer-macos) ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined CC=/usr/local/opt/llvm/bin/clang CXX=/usr/local/opt/llvm/bin/clang++ --disable-asm && make clean && make -j5
...
...
CXXLD test/fuzz/uint256_deserialize
Making all in doc/man
make[1]: Nothing to be done for `all'.
make[1]: Nothing to be done for `all-am'.
```
ACKs for top commit:
fanquake:
ACK c8992e8959 - tested that compiling works on macOS.
MarcoFalke:
review ACK c8992e8959
Tree-SHA512: 965cdc61b30db0e2209c91b29f0d42de927a9a5b85e1e70f22d1452e0955f876726c7a8c1d1a5f448f12bf24eec3000802071cd4ae28d8605343fd43d174ca84
c0f09c2c9d fuzz: add missing overrides to signature_checker (Jon Atack)
Pull request description:
These functions in `fuzz/signature_checker.cpp` override virtual member functions and should be marked `override` instead of `virtual`, which is for introducing a new virtual function. The overridden virtual functions are in `script/interpreter.h:151/156/161`.
Also, per MarcoFalke suggestion, add missing parentheses in `fuzz/scriptnum_ops.cpp` and remove useless `unsigned int >= 0` conditional in `fuzz/script.cpp`.
These changes fix 5 compile warnings in gcc 10 and 3 in clang 11/12.
ACKs for top commit:
vasild:
ACK c0f09c2
MarcoFalke:
review ACK c0f09c2c9d
Tree-SHA512: 76ce73ec577c1f23cf8646c31d44dcd6c6303732c47187d041a8921d0d24a50163989a375352ebc221abf2ac337bc0902149be46b6f9eebc071d2f364c407f71
and also
- add missing parentheses in fuzz/scriptnum_ops.cpp
- remove useless unsigned int conditional in fuzz/script.cpp
These changes fix 5 compile warnings in gcc 10.
4455949d6f Make test DoS_mapOrphans deterministic (David Reikher)
Pull request description:
This pull request proposes a solution to make the test `DoS_mapOrphans` in denialofservice_tests.cpp have deterministic coverage.
The `RandomOrphan` function in denialofservice_tests.cpp and the implicitly called function `ecdsa_signature_parse_der_lax` in pubkey.cpp were causing the non-deterministic test coverage.
In the former, if a random orphan was selected the index of which is bigger than the max. orphan index in `mapOrphanTransactions`, the last orphan was returned from `RandomOrphan`. If the random number generated was never large enough, this condition would not be fulfilled and the corresponding branch wouldn't run. The proposed solution is to force one of the 50 dependant orphans to depend on the last orphan in `mapOrphanTransactions` using the newly introduced function `OrphanByIndex` (and passing it a large uint256), forcing this branch to run at least once.
In the latter, if values for ECDSA `R` or `S` (or both) had no leading zeros, some code would not be executed. The solution was to find a constant signature that would be comprised of `R` and `S` values with leading zeros and calling `CPubKey::Verify` at the end of the test with this signature forcing this code to always run at least once at the end even if it hadn't throughout the test.
To test that the coverage is (at least highly likely) deterministic, I ran
`contrib/devtools/test_deterministic_coverage.sh denialofservice_tests/DoS_mapOrphans 1000`
and the result was deterministic coverage across 1000 runs.
Also - removed denialofservice_tests test entry from the list of non-deterministic tests in the coverage script.
ACKs for top commit:
MarcoFalke:
ACK 4455949d6f
Tree-SHA512: 987eb1f94b80d5bec4d4944e91ef43b9b8603055750362d4b4665b7f011be27045808aa9f4c6ccf8ae009b61405f9a1b8671d65a843c3328e5b8acce1f1c00a6
The RandomOrphan function and the function ecdsa_signature_parse_der_lax
in pubkey.cpp were causing non-deterministic test coverage.
Force seed in the beginning of the test to make it deterministic.
The seed is selected carefully so that all branches of the function
ecdsa_signature_parse_der_lax are executed. Prior to this fix, the test
was exhibiting non-deterministic coverage since none of the ECDSA
signatures that were generated during the test had leading zeroes in
either R, S, or both, resulting in some branches of said function not
being executed. The seed ensures that both conditions are hit.
Removed denialofservice_tests test entry from the list of non-deterministic
tests in the coverage script.
ad6c34881d tests: Add fuzzing harness for CBlockPolicyEstimator::{Read,Write} (policy/fees.h) (practicalswift)
614e0807a8 tests: Add fuzzing harness for CBufferedFile::{SetPos,GetPos,GetType,GetVersion} (stream.h) (practicalswift)
7bcc71e5f8 tests: Add fuzzing harness for LoadExternalBlockFile(...) (validation.h) (practicalswift)
9823376030 tests: Add fuzzing harness for CBufferedFile (streams.h) (practicalswift)
f3aa659be6 tests: Add fuzzing harness for CAutoFile (streams.h) (practicalswift)
e507c0799d tests: Add serialization/deserialization fuzzing helpers WriteToStream(…)/ReadFromStream(…) (practicalswift)
e48094a506 tests: Add FuzzedAutoFileProvider which provides a CAutoFile interface to FuzzedDataProvider (practicalswift)
9dbcd6854c tests: Add FuzzedFileProvider which provides a FILE* interface to FuzzedDataProvider using fopencookie (practicalswift)
Pull request description:
Add fuzzing harnesses for `CAutoFile`, `CBufferedFile`, `LoadExternalBlockFile` and other `FILE*` consumers:
* Add `FuzzedFileProvider` which provides a `FILE*` interface to `FuzzedDataProvider` using `fopencookie`
* Add `FuzzedAutoFileProvider` which provides a `CAutoFile` interface to `FuzzedDataProvider`
* Add serialization/deserialization fuzzing helpers `WriteToStream(…)`/`ReadFromStream(…)`
* Add fuzzing harness for `CAutoFile` (`streams.h`)
* Add fuzzing harness for `CBufferedFile` (`streams.h`)
* Add fuzzing harness for `LoadExternalBlockFile(...)` (`validation.h`)
* Add fuzzing harness for `CBlockPolicyEstimator::Read` and `CBlockPolicyEstimator::Write` (`policy/fees.h`)
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
Crypt-iQ:
Tested ACK ad6c348
Tree-SHA512: a38e142608218496796a527d7e59b74e30279a2815450408b7c27a76ed600cebc6b88491e831665a0639671e2d212453fcdca558500bbadbeb32b267751f8f72
f58c4b538e [tests] Remove unnecessary cs_mains in denialofservice_tests (Matt Corallo)
Pull request description:
9fdf05d70c resolved some lock
inversion warnings in denialofservice_tests, but left in a number
of cs_main locks that are unnecessary (introducing lock inversion
warnings in future changes).
ACKs for top commit:
promag:
ACK f58c4b538e.
jonatack:
ACK f58c4b538e verified the test locks correspond to the locks in net/net_processing, and the debug build is clean/unit tests pass.
Tree-SHA512: de2d9b2a8f08081b2ce31e18585e4677b167a11752b797d790c281575d7dfef3587f8be4fc7f8f16771141b6ff0b0145c7488cf30e79256b0043947c67a6182c
0c8461a88e refactor: replace CConnman pointers by references in net_processing.cpp (Sebastian Falbesoner)
Pull request description:
This is a follow-up to the recently merged PR https://github.com/bitcoin/bitcoin/pull/19053, replacing ~~two more types of~~ one more type of pointer (CConnman) by references to increase the code quality -- pointers should either check for `nullptr` or be replaced by references, and the latter strategy seems to be more reasonable.
Again, to keep the review burden managable, the changes are kept simple,
* only tackling `CConnman*` ~~and `BanMan*`~~ pointers
* only within the net_processing module, i.e. no changes that would need adaption in other modules
* keeping the names of the variables as they are
ACKs for top commit:
jnewbery:
utACK 0c8461a88e
MarcoFalke:
ACK 0c8461a88e🕧
Tree-SHA512: 79dc05144bcfb5e0bbc62180285aadcc6199f044fa3016c0f54f7b7f45037415260970037bd63b18fafefb8aef448549dae14b780bafb540fa2373f493a17f71
9fdf05d70c resolved some lock
inversion warnings in denialofservice_tests, but left in a number
of cs_main locks that are unnecessary (introducing lock inversion
warnings in future changes).
fa108d6a75 test: update tests for peer discouragement (Jon Atack)
1a9f462caa gui, doc: rm Ban Score in GUI Peers window/release notes updates (Jon Atack)
Pull request description:
This is the third `-banscore` PR in the mini-series described in #19464. See that PR for the intention and reasoning.
- no longer display "Ban Score" in the GUI peers window and add a release note, plus release note fixups per https://github.com/bitcoin/bitcoin/pull/19464#pullrequestreview-447452052
- update tests (`src/test/denialofservice_tests.cpp` and `test/functional/p2p_leak.py`) from banning to discouragement and per https://github.com/bitcoin/bitcoin/pull/19464#issuecomment-658052518
ACKs for top commit:
jnewbery:
ACK fa108d6a75
laanwj:
ACK fa108d6a75
Tree-SHA512: 58a449b3f47b8cb5490b34e4442ee8675bfad1ce48af4e4fd5c67715b0c1a596fb8e731d42e576b4c3b64627f76e0a68cbb1da9ea9f588a5932fe119baf40d50
cca7c577d5 tests: Add fuzzing harness for ChaCha20Poly1305AEAD (practicalswift)
2fc4e5916c tests: Add fuzzing harness for ChaCha20 (practicalswift)
e9e8aac029 tests: Add fuzzing harness for CHKDF_HMAC_SHA256_L32 (practicalswift)
ec86ca1aaa tests: Add fuzzing harness for poly1305_auth(...) (practicalswift)
4cee53bba7 tests: Add fuzzing harness for AES256CBCEncrypt/AES256CBCDecrypt (practicalswift)
9352c32325 tests: Add fuzzing harness for AES256Encrypt/AES256Decrypt (practicalswift)
Pull request description:
Add fuzzing harness for `AES{CBC,}256{Encrypt,Decrypt}`, `poly1305_auth`, `CHKDF_HMAC_SHA256_L32`, `ChaCha20` and `ChaCha20Poly1305AEAD`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
laanwj:
ACK cca7c577d5
Tree-SHA512: cff9acefe370c12a3663aa55145371df835479c6ab8f6d81bbf84e0f81a9d6b0d94e45ec545f9dd5e1702744eaa7947a1f4ffed0171f446fc080369161afd740
fa53635381 util: Make Assert work with any value (MarcoFalke)
Pull request description:
Goal is to avoid compile failures
ACKs for top commit:
jonatack:
ACK fa53635381
ryanofsky:
Code review ACK fa53635381. Looks like if argument is an lvalue this effectively does:
Tree-SHA512: a5cf47a8bb2fa1bd8b8895774f33de50ad803165d6f7b520351be1cfcd5612d5d97c51d118461331d30640186c470879e5ad19e3333e09e72685c5e4e4f23079
Qt tests currently are currently using two NodeContext structs at the same
time, one in interfaces::NodeImpl::m_context, and the other in
BasicTestingSetup::m_node, and the tests have hacks transferring state between
them.
Fix this by getting rid of the NodeImpl::m_context struct and making it a
pointer. This way a common BitcoinApplication object can be used for all qt
tests, but they can still have their own testing setups.
Non-test code is changing but non-test behavior is still the same as before.
Motivation for this PR is to be able to remove the
"std::move(test.m_node.connman)" and mempool hacks for swapping individual
NodeContext members in Qt tests, because followup PR #19099 adds yet another
member (wallet_client) that needs to be swapped. After this change, the whole
NodeContext struct can be swapped instead of individual members, so the
workarounds are less fragile and invasive.
