Age | Commit message (Collapse) | Author |
|
when activating snapshot
fa996c58e8a31ebe610d186cef408b6dd3b385a8 refactor: Avoid integer overflow in ApplyStats when activating snapshot (MarcoFalke)
fac01888d17423d6c23a9ce15d98fc88fb34e3cc Move AdditionOverflow to util, Add CheckedAdd with unit tests (MarcoFalke)
fa526d8fb6ab8f2678a30d4536aa9c45218f5269 Add dev doc to CCoinsStats::m_hash_type and make it const (MarcoFalke)
faff051560552d4405896e01920a18f698155a56 style: Remove unused whitespace (MarcoFalke)
Pull request description:
A snapshot contains the utxo set, including the out value. To activate the snapshot, the hash needs to be calculated. As a side-effect, the total amount in the snapshot is calculated (as the sum of all out values), but never used. Instead of running into an integer overflow in an unused result, don't calculate the result in the first place.
Other code paths (using the active utxo set) can not run into an integer overflow, since the active utxo set is valid.
Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39716
ACKs for top commit:
shaavan:
reACK fa996c58e8a31ebe610d186cef408b6dd3b385a8
vasild:
ACK fa996c58e8a31ebe610d186cef408b6dd3b385a8
Tree-SHA512: 4f207f634841f6f634fd02ae1e5907e343fd767524fd0e8149aa99fa9a1834fe50167d14874834d45236e9c325d567925f28129bacb7d80be29cf22277a16a14
|
|
interface, extend coverage
ea4c9fd4ab9aaa2e8f2c2e38a75c9f05d0bfc866 test: Cover eviction by timeout in addrman_evictionworks (Martin Zumsande)
4f1bb467b556ec93c9b8f758783fda4d050da491 test: Add test for multiplicity in addrman new tables (Martin Zumsande)
e880bb7836dab2018049390884220177c6db9b92 test: Add test for updating addrman entries (Martin Zumsande)
f02eee8c8784dfc8db80a21ab6508f7c99298255 test: introduce utility function to retrieve an addrman (Martin Zumsande)
f0e5efb82493f7a14580335ce719d5be81c8713e test: Remove unused AddrManTest class (Martin Zumsande)
b696d7870b29232057600df5ddd8351888253b95 test: Remove tests for internal helper functions (Martin Zumsande)
0538520091bf2982a029a0298835400f5afbdc15 test: use AddrMan instead of AddrManTest where possible (Martin Zumsande)
1c65d427bbf61bb558cf7e18f7aff99b19f68508 test: Inline SimConnFail function (Martin Zumsande)
5b7aac34f2363822c3a1cfafda8ffc9528905058 test: delete unused GetBucketAndEntry function (Amiti Uttarwar)
2ba1e74e59a325ca6cb140757067dd5e0c7c249b test: Update addrman_serialization unit test to use AddrMan's interface (Amiti Uttarwar)
dad5f760211df314d650999e0a76edb0151b4fe1 addrman: Introduce a test-only function to lookup addresses (Amiti Uttarwar)
Pull request description:
This PR (joint work with Amiti Uttarwar) changes the addrman unit tests such that they only use the public `AddrMan` interface:
This has the advantage that the tests are less implementation-dependent, i.e. it would be possible to rewrite the internal addrman implementation (as drafted [here](https://github.com/sipa/bitcoin/tree/202106_multiindex_addrman) for using a multiindex) without having to adjust the tests.
This includes the following steps:
* Adding a test-only function `FindAddressEntry()` to the public addrman interface which returns info about an address in addrman (e.g. bucket, position, whethe the address is in new or tried). Obviously we want to do this sparingly, but I think a single test-only function is ok (which could also be useful elsewhere, e.g. in fuzz tests).
* Removal of the `AddrManTest` subclass which would reach into AddrMan's internals, using `AddrMan` instead
* Removal of tests for internal helper functions that are not publicly exposed (these are still tested indirectly via the public functions calling them).
* Additional tests for previously untested features such as multiplicity in the new tables, that can be tested with the help of `FindAddressEntry()`.
All in all, this PR increases the unit test coverage of AddrMan by a bit.
ACKs for top commit:
jnewbery:
ACK ea4c9fd4ab9aaa2e8f2c2e38a75c9f05d0bfc866
josibake:
reACK https://github.com/bitcoin/bitcoin/pull/23826/commits/ea4c9fd4ab9aaa2e8f2c2e38a75c9f05d0bfc866
Tree-SHA512: c2d4ec8bdc62ffd6055ddcd37dea85ec08c76889e9e417e8d7c62a96cf68a8bcbe8c67bec3344d91fa7d3c499f6d9f810962da1dddd38e70966186b10b8ab447
|
|
|
|
|
|
|
|
This switches .read() and .write() to take spans of bytes.
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
* 2020: fa0074e2d82928016a43ca408717154a1c70a4db
* 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
This covers Connected() which updates nTime, and SetServices()
which updates nServices
|
|
|
|
|
|
The logic of these functions is already covered by existing unit tests
using publicly exposed functions of the interface.
Therefore, removing them does not decrease test coverage.
|
|
Switches to AddrMan for tests that use no features of AddrManTest.
Also removes unusued AddrManTest variables
Co-Authored-By: Amiti Uttarwar <amiti@uttarwar.org>
|
|
No need for a function, since it is only used once.
Co-Authored-By: Amiti Uttarwar <amiti@uttarwar.org>
|
|
|
|
By updating the test to use FindEntry, it no longer needs to reach into
AddrMan's internals (via GetBucketAndEntry) to assert expected
behaviors.
|
|
|
|
|
|
|
|
|
|
ChainTestingSetup
826e12b010eda4238f9e8cd875e8915a405bed0d test: call VerifyLoadedChainstate during ChainTestingSetup (James O'Beirne)
Pull request description:
for additional coverage and similarity to actual init process.
Followup to #23280.
ACKs for top commit:
dongcarl:
Code Review ACK 826e12b010eda4238f9e8cd875e8915a405bed0d
ryanofsky:
Code review ACK 826e12b010eda4238f9e8cd875e8915a405bed0d
Tree-SHA512: a4e7fd25e5d7a08b1e154ae6daf67c3048260a2684b0e569b544dd826693b7b969db9923b191e499cb8d8d0a2a73eb9330ff45909313145a9abb6052eb8c3ad9
|
|
use output from `AddrMan::Good()`
bf4f8171352e9b384b42c91da61dfc9c3ca89ed8 refactor: addrman_select test (josibake)
5a64dc018c04ce16202a8e58ce92d2657c0b1806 refactor: addrman_evictionworks test (josibake)
e281fccd8a80d7cd48c3b17d58fd4a8915e1e965 refactor: addrman_noevict test (josibake)
8bdd9240d4310aafa1332159355f106a8fcfc5c9 refactor: addrman_selecttriedcollisions test (josibake)
Pull request description:
As a follow-up to #23713 , this PR refactors the remaining tests in `src/tests/addrman_tests.cpp` to use the output from `AddrMan::Good()` where appropriate.
ACKs for top commit:
naumenkogs:
ACK bf4f8171352e9b384b42c91da61dfc9c3ca89ed8
mzumsande:
Code Review ACK bf4f8171352e9b384b42c91da61dfc9c3ca89ed8
Tree-SHA512: 93cc127aecff42c1c174daa04911af7e3460a5c40ddf96952fe4a6ab86fa1ff22d66724326abb709008d7f9f79c26c55c6d62753c40059c9ac60f869507ec913
|
|
c44c20108f7b7314f59f034110789385a24db280 p2p, refactor: drop unused DNSLookupFn param in LookupSubnet() (Vasil Dimov)
f0c9e68080432c1ab11b14e571b8dfb7cfe727f8 p2p, refactor: tidy up LookupSubNet() (Jon Atack)
Pull request description:
This pull originally resolved a code `TO-DO`, as well as fixing different param names between the function declaration and definition, updating the function to current style standards, clearer variable naming, and improving the Doxygen documentation.
Following the merge of #17160, it now does the non-`TODO` changes and also now drops an unused param to simplify the function.
ACKs for top commit:
dunxen:
ACK c44c201
vasild:
ACK c44c20108f7b7314f59f034110789385a24db280
shaavan:
crACK c44c20108f7b7314f59f034110789385a24db280
Tree-SHA512: 55f64c7f403819dec84f4da06e63db50f7c0601a2d9a1ec196fda667c220ec6f5ad2a3c95e0e02275da9f6da6b984275d1dc10e19ed82005c5e13da5c5ecab02
|
|
edd0313ae7c94420642081c9172e349080bb9335 test: Improve "invalid_command" subtest in system_tests for Windows (Hennadii Stepanov)
fb1b0590af138e317803893d2cab9dc887f33c5b test: Fix "non-zero exit code" subtest in system_tests for Windows (Hennadii Stepanov)
0aad33db6410ed36fa0f4b96245cacbae7897d2e test: Fix "false" subtest in system_tests for Windows (Hennadii Stepanov)
507c009c1ee68a4c3ad100f765bf854307d5bf39 test: Fix "echo" subtest in the system_tests for Windows (Hennadii Stepanov)
Pull request description:
An attempt to fix bitcoin/bitcoin#23775.
With this PR on Windows 10 Pro 21H1 (build 19043.1348):
```
C:\Users\hebasto\bitcoin>src\test_bitcoin.exe --run_test=system_tests/run_command
Running 1 test case...
*** No errors detected
C:\Users\hebasto\bitcoin>src\test_bitcoin.exe
Running 482 test cases...
*** No errors detected
```
ACKs for top commit:
sipsorcery:
tACK edd0313ae7c94420642081c9172e349080bb9335
Tru3Nrg:
tACK https://github.com/bitcoin/bitcoin/pull/23781/commits/edd0313ae7c94420642081c9172e349080bb9335
Tree-SHA512: 66a4f2372858011ff862b71c6530bedb8bc731b18595636fac9affc9189d9320f212c68b62498f2b57ee7a07f59e842dbec085b76a7419791d1a06c8e80e7744
|
|
|
|
Core's and D. J. Bernstein's implementation of ChaCha20
4d0ac72f3ae78e3c6a0d5dc4f7e809583abd0546 [fuzz] Add fuzzing harness to compare both implementations of ChaCha20 (stratospher)
65ef93203cc6a977c8e96f07cb9155f46faf5004 [fuzz] Add D. J. Bernstein's implementation of ChaCha20 (stratospher)
Pull request description:
This PR compares Bitcoin Core's implementation of ChaCha20 with D. J. Bernstein's in order to find implementation discrepancies if any.
ACKs for top commit:
laanwj:
Code review ACK 4d0ac72f3ae78e3c6a0d5dc4f7e809583abd0546
Tree-SHA512: f826144b4db61b9cbdd7efaaca8fa9cbb899953065bc8a26820a566303b2ab6a17431e7c114635789f0a63fbe3b65cb0bf2ab85baf882803a5ee172af4881544
|
|
|
|
for packages of 1 child + parents
046e8ff264be6b888c0f9a9d822e32aa74e19b78 [unit test] package submission (glozow)
e12fafda2dfbbdf63f125e5af797ecfaa6488f66 [validation] de-duplicate package transactions already in mempool (glozow)
8310d942e046c5a9b6bd90afdcd3af68dd91e081 [packages] add sanity checks for package vs mempool limits (glozow)
be3ff151a1f9665720cdf70d072b098a2f9726a9 [validation] full package accept + mempool submission (glozow)
144a29099a865ac1dc3e5291d9529fbcca9c83a4 [policy] require submitted packages to be child-with-unconfirmed-parents (glozow)
d59ddc5c3d1c035474d7bc9fa9f8a0eeb1c8498c [packages/doc] define and document package rules (glozow)
ba26169f6035c238378a3c9647213328a006fa23 [unit test] context-free package checks (glozow)
9b2fdca7f03911ac40fe0f8a0b5da534bee4554b [packages] add static IsChildWithParents function (glozow)
Pull request description:
This is 1 chunk of [Package Mempool Accept](https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a); it restricts packages to 1 child with its parents, doesn't allow conflicts, and doesn't have CPFP (yet). Future PRs (see #22290) will add RBF and CPFP within packages.
ACKs for top commit:
laanwj:
Code review ACK 046e8ff264be6b888c0f9a9d822e32aa74e19b78
Tree-SHA512: 37dbba37d527712f8efef71ee05c90a8308992615af35f5e0cfeafc60d859cc792737d125aac526e37742fe7683ac8c155ac24af562426213904333c01260c95
|
|
Check that `Good()` is successful whenever it is called.
|
|
Test for collisions and duplicates directly with `Good()`.
If an entry to tried is a duplicate, `Good()` will return false
but `SelectTriedCollision()` will be empty (assuming there were no prior
collisions). If there is a collision, `Good()` will retun false
and `SelectTriedCollision()` will return a value.
|
|
Check the response from `Good()` wherever it is called.
Previously, the test was using `size()` (incorrect for checking tried)
and `SelectTriedCollision()` to determine if a collision happened.
|
|
Check `Good()` directly when adding addresses.
Previously, test would check `size()`, which is incorrect.
Check that duplicates are also handled by checking the
output from `SelectTriedCollision()` when `Good()` returns
false.
|
|
No need to explain code with comments.
|
|
|
|
|
|
|
|
connection time
fad943821e35d0eb2143061e718f0193e12a4c71 scripted-diff: Rename touched member variables (MarcoFalke)
fa663a4c0d20487ed3f7a93e1c2ca9932b05f5a8 Use mockable time for peer connection time (MarcoFalke)
fad7ead146a152f46b25ce6623e05cbb1dbc8cca refactor: Use type-safe std::chrono in net (MarcoFalke)
Pull request description:
Benefits:
* Type-safe
* Mockable
* Allows to revert a temporary test workaround
ACKs for top commit:
naumenkogs:
ACK fad943821e35d0eb2143061e718f0193e12a4c71
shaavan:
ACK fad943821e35d0eb2143061e718f0193e12a4c71
Tree-SHA512: af9bdfc695ab727b100c6810a7289d29b02b0ea9fa4fee9cc1f3eeefb52c8c465ea2734bae0c1c63b3b0d6264ba2c493268bc970ef6916570eb166de77829d82
|
|
faa6c3d44c861c0486c1369e1d098b7645ab07cd net: Drop only invalid entries when reading banlist.json (MarcoFalke)
Pull request description:
All entries will be dropped when there is at least one invalid one in `banlist.json`. Fix this by only dropping invalid ones.
Also suggested in https://github.com/bitcoin/bitcoin/pull/20966#issuecomment-861150204
ACKs for top commit:
laanwj:
Re-ACK faa6c3d44c861c0486c1369e1d098b7645ab07cd
Tree-SHA512: 5a58e7f1dcabf78d0c65d8c6d5d997063af1efeaa50ca7730fc00056fda7e0061b6f7a38907ea045fe667c9f61d392e01e556b425a95e6b126e3c41cd33deb83
|
|
snapshot use
2283b9cd1ee0fbd1e8ebc61673b1fe7596199a24 test: add tests for LoadBlockIndex when using multiple chainstates (James O'Beirne)
0fd599a51a700c028d6f7ed8067d2d9f6e6a04a5 validation: have LoadBlockIndex account for snapshot use (James O'Beirne)
d0c6e61f5dd3b6af818459d9d03b7ba316c5a3f7 validation: don't modify genesis during snapshot load (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Currently, `BlockManager::LoadBlockIndex` adds all blocks that have downloaded transactions to the active chain state's `setBlockIndexCandidates` set, ignoring the background chain state.
This PR changes ChainstateManager::LoadBlockIndex to update `setBlockIndexCandidates` in the background chain, not just the active chain. In the active chain, the same blocks are added as before. In the background chain, only blocks that have actually been validated, not blocks marked assumed-valid are added so the background chain will continue to download and validate assumed-valid blocks.
ACKs for top commit:
MarcoFalke:
Concept ACK 2283b9cd1ee0fbd1e8ebc61673b1fe7596199a24 🤽
Sjors:
utACK 2283b9cd1ee0fbd1e8ebc61673b1fe7596199a24
Tree-SHA512: 7c9a80802df4722d85d12b78d2e7f628ac5f11cb8be66913d5c3230339bd1220c6723805509d4460826a17d1dc04b0ae172eb7d09ac0ea5dc5e41d77975cbd5e
|
|
addrman_tried_collisions test to directly check for collisions
caac999ff0fc5c98fa438b7e96fe1232f6573fd5 refactor: remove dependence on AddrManTest (josibake)
f961c477b56737c546c275e4d86cecfa3f75d48c refactor: check Good() in tried_collisions test (josibake)
207f1c825c632c54af009516d376d392ea9106fa refactor: make AddrMan::Good return bool (josibake)
Pull request description:
Previously, the `addrman_tried_collisions` test behaved in the following way:
1. add an address to addrman
2. attempt to move the new address to the tried table (using `AddrMan.Good()`)
3. verify that `num_addrs` matched `size()` to check for collisions in the new table
`AddrMan.size()`, however, returns the number of unique address in addrman, regardless of whether they are in new or tried. This means the test would still pass for addresses where a collision did occur in the tried table. After 3 collisions in the tried table, there would eventually be a collision in the new table when trying to add a new address, which was then detected by checking `num_addrs - collisions == size()`.
While the collision in the new table was caused by a collision in the tried table, the test is misleading as it's not directly testing for collisions in the tried table and misses 3 collisions before identifying a collision in the new table.
### solution
To more directly test the tried table, I refactored `AddrMan::Good()` to return a boolean after successfully adding an address to the tried table. This makes the test much cleaner by first adding an address to new, calling `Good` to move it to the tried table, and checking if it was successful or not. It is worth noting there are other reasons, aside from collisions, which will cause `Good` to return false. That being said, this is an improvement over the previous testing methodology.
Additionally, having `Good()` return a boolean is useful outside of testing as it allows the caller to handle the case where `Good` is unable to move the entry to the tried table (e.g https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/rpc/net.cpp#L945).
### followup
As a follow up to this PR, I plan to look at the following places `Good()` is called and see if it makes sense to handle the case where it is unable to add an entry to tried:
* https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/rpc/net.cpp#L945
* https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/net.cpp#L2067
* https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/net_processing.cpp#L2708
ACKs for top commit:
jnewbery:
utACK caac999ff0
mzumsande:
Code review ACK caac999ff0fc5c98fa438b7e96fe1232f6573fd5
Tree-SHA512: f328896b1f095e8d2581fcdbddce46fc0491731a0440c6fff01081fa5696cfb896dbbe1d183eda2c100f19aa111e1f8b096ef93582197edc6b791de563a58f99
|
|
Currently all entries in the file are dropped. Fix that by only dropping the invalid ones
|
|
|
|
Rather than try to infer a collision by checking `AddrMan::size`,
check whether or not moving to the tried table was successful by
checking the output from `AddrMan::Good`
|
|
fa19bab90a3ccc2f76c20aa805292d6a9c5d8071 fuzz: Rework FillNode (MarcoFalke)
fae6e31df7c6df04f41fc8401e2a9781a4d75be7 refactor: Set fSuccessfullyConnected in FillNode (MarcoFalke)
fa3583f856e34b6c6134745da14f5206cf71fa3e fuzz: Avoid negative NodeId in ConsumeNode (MarcoFalke)
Pull request description:
Currently `FillNode` is a bit clumsy because it directly modifies memory of `CNode`. This gets in the way of moving that memory to `Peer`. Also, it isn't particularly consistent. See for example https://github.com/bitcoin/bitcoin/pull/21160#discussion_r739206139 .
Fix all issues by sending a `version`/`verack` in `FillNode` and let net_processing figure out the internal details.
ACKs for top commit:
jnewbery:
Strong concept ACK and light code review ACK fa19bab90a3ccc2f76c20aa805292d6a9c5d8071
Tree-SHA512: 33261d857c3fa6d5d39d742624009a29178ad5a15eb3fd062da741affa5a4854fd45ed20d59a6bba2fb068cf7b39cad6f95b2910be7cb6afdc27cd7917955b67
|
|
Incorporates feedback from Russ Yanofsky.
|
|
Avoid modifying the genesis block index entry during snapshot load. This
is because, in a future change that fixes LoadBlockIndex for UTXO
snapshots, we detect block index entries that are reliant on
assumed-valid ancestors and treat them specially.
Since the genesis block doesn't have BLOCK_VALID_SCRIPTS, it would be
erroneously marked BLOCK_ASSUMED_VALID during snapshot load if we didn't
skip it here. This would cause a "setBlockIndexCandidates() empty"
assertion to be tripped since all block index entries would be marked
assume-valid due to genesis, which is never re-validated.
There's probably no good reason to modify the genesis block index entry
during snapshot load anyway...
|
|
for additional coverage and similarity to actual init process.
|
|
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; }
ren nLastBlockTime m_last_block_time
ren nLastTXTime m_last_tx_time
ren nTimeConnected m_connected
-END VERIFY SCRIPT-
|
|
This allows to revert the temporary commit
0bfb9208df556f77cddfedfd73e5811a0e031d34 (test: fix test failures in
test/functional/p2p_timeouts.py).
|
|
|
|
fa72dd314fe857d827d9b311bdf0453e9463746f fuzz: Move ISO8601 to one place (MarcoFalke)
Pull request description:
Seems confusing to split this to two places.
Also fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=42178
ACKs for top commit:
fanquake:
ACK fa72dd314fe857d827d9b311bdf0453e9463746f
Tree-SHA512: 637b0671078848ea417fdf66b92715602040fad34d4ca5f7b843a519a1cfeebe5d992a79a399deba39926905125681d66ab0dc05f66f79a26f3bf555e12fb0ba
|