Age | Commit message (Collapse) | Author |
|
|
|
|
|
fa45a1338adb127d69aee982920e29519bc1fed6 refactor: Remove unused validation includes (MarcoFalke)
Pull request description:
Unused includes will cause needless recompilation when headers are changed. Also, they pretend there are dependencies that don't exist.
Fix both by removing them.
ACKs for top commit:
laanwj:
Code review ACK fa45a1338adb127d69aee982920e29519bc1fed6
theStack:
ACK fa45a1338adb127d69aee982920e29519bc1fed6 ♻️
Tree-SHA512: 69190fd09184b75bce34ce3f315a1817e09ea32779f9ddc2d4790c89b0887b6cebd88aba66fa054c43c9183fc66202a556d982dd7034fc389a75802d8aaac83a
|
|
Can be reviewed with -W --ignore-all-space
Fixes:
* Calling ConsumeRandomLengthByteVector 4 times, when 2 is enough.
* Slow execution speed: Finalize is expensive because it invokes
division. Speed up the target by calling Finalize() at most twice per
fuzz input.
|
|
Can be reviewed with --color-moved=dimmed-zebra
|
|
|
|
parsing using std::from_chars(…) (C++17)
util: Avoid locale dependent functions strtol/strtoll/strtoul/strtoull in ParseInt32/ParseInt64/ParseUInt32/ParseUInt64
fuzz: Assert equivalence between new and old Parse{Int,Uint}{8,32,64} functions
test: Add unit tests for ToIntegral<T>(const std::string&)
|
|
|
|
After the performance improvement for Good(), the direct method is only 2x faster
as opposed to 60x before.
|
|
corrupted
fa55c3dc1b4bbdc6a53bd11fa6c0b2ec6bbb64ae Raise InitError when peers.dat is invalid or corrupted (MarcoFalke)
fa4e2ccfd8ae96c381947285bef47cb39474ac89 Inline ReadPeerAddresses (MarcoFalke)
fa5aeec80c6cdca9ca027d80dff3b397911ff2c2 Move LoadAddrman from init to addrdb (MarcoFalke)
Pull request description:
peers.dat is silently erased when it can not be parsed or when it appears corrupted. Fix that by notifying the user. This might help in the following examples:
* The user provided the database, but picked the wrong one.
* A future version of Bitcoin Core wrote the file and it can't be read.
* The file was corrupted by a logic bug in Bitcoin Core.
* The file was corrupted by a disk failure.
ACKs for top commit:
jonatack:
Code review re-ACK fa55c3dc1b4bbdc6a53bd11fa6c0b2ec6bbb64ae per `git range-diff eb1f570 fa59c6d fa55c3` and verified the new tests fail on master, except "Check mocked addrman is valid", as expected
prayank23:
tACK https://github.com/bitcoin/bitcoin/commit/fa55c3dc1b4bbdc6a53bd11fa6c0b2ec6bbb64ae
vasild:
ACK fa55c3dc1b4bbdc6a53bd11fa6c0b2ec6bbb64ae
Tree-SHA512: 78264a78ee570a3c3262cf9c8542b5ffaffa5f52da1eef66c8c381f346989272967cfe1769c573502d9d7d3f7ad68c3ac3b2ec734185d2e4e7595b7122b14196
|
|
853c4edb70f897a6a7165abaea4a303d7d448721 [net] Remove asmap argument from CNode::CopyStats() (John Newbery)
9fd5618610e91e3949536c5122cf31eb58c9aa6b [asmap] Make DecodeAsmap() a utility function (John Newbery)
bfdf4ef334a16ef6108a658bf4f8514754128c18 [asmap] Remove SanityCheckASMap() from netaddress (John Newbery)
07a9eccb60485e71494664cc2b1964ae06a3dcf0 [net] Remove CConnman::Options.m_asmap (John Newbery)
Pull request description:
These small cleanups to the asmap code are the first 4 commits from #22910. They're minor improvements that are independently useful whether or not 22910 is merged.
ACKs for top commit:
naumenkogs:
ACK 853c4edb70f897a6a7165abaea4a303d7d448721
theStack:
Concept and code-review ACK 853c4edb70f897a6a7165abaea4a303d7d448721 🗺️
fanquake:
ACK 853c4edb70f897a6a7165abaea4a303d7d448721
Tree-SHA512: 64783743182592ac165df6ff8d18870b63861e9204ed722c207fca6938687aac43232a5ac4d8228cf8b92130ab0349de1b410a2467bb5a9d60dd9a7221b3b85b
|
|
|
|
This saves passing around a reference to the asmap std::vector<bool>.
|
|
SanityCheckASMap(asmap, bits) simply calls through to SanityCheckASMap(asmap)
in util/asmap. Update all callers to simply call that function.
|
|
The class only stores the file path, reading it from a global. Globals
are confusing and make testing harder.
The method reading from a stream does not even use any class members, so
putting it in a class is also confusing.
|
|
|
|
This is a follow-up to the code move in commit a820e79512b67b1bfda20bdc32b47086d2b0910d
|
|
724c4975622bc22cedc3f3814dfc8e66cf8371f7 [fuzz] Add ConsumeAsmap() function (John Newbery)
5840476714ffebb2599999c85a23b52ebcff6090 [addrman] Make m_asmap private (John Newbery)
f9002cb5dbd573cd9ca200de21319fa296e26055 [net] Rename the copyStats arg from m_asmap to asmap (John Newbery)
f572f2b2048994b3b50f4cfd5de19e40b1acfb22 [addrman] Set m_asmap in CAddrMan initializer list (John Newbery)
593247872decd6d483a76e96d79433247226ad14 [net] Remove CConnMan::SetAsmap() (John Newbery)
50fd77045e2f858a53486b5e02e1798c92ab946c [init] Read/decode asmap before constructing addrman (John Newbery)
Pull request description:
Commit 181a1207 introduced an initialization order bug: CAddrMan's m_asmap must be set before deserializing peers.dat.
The first commit restores the correct initialization order. The remaining commits make `CAddrMan::m_asmap` usage safer:
- don't reach into `CAddrMan`'s internal data from `CConnMan`
- set `m_asmap` in the initializer list and make it const
- make `m_asmap` private, and access it (as a reference to const) from a getter.
This ensures that peers.dat deserialization must happen after setting m_asmap, since m_asmap is set during CAddrMan construction.
ACKs for top commit:
mzumsande:
Tested ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7
amitiuttarwar:
code review but utACK 724c497562
naumenkogs:
utACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7
vasild:
ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7
MarcoFalke:
review ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7 👫
Tree-SHA512: 684a4cf9e3d4496c9997fb2bc4ec874809987055c157ec3fad1d2143b8223df52b5a0af787d028930b27388c8efeba0aeb2446cb35c337a5552ae76112ade726
|
|
Improve readability of code, simplify future scripted diff cleanup PRs, and be
more consistent with naming for GetBoolArg.
This will also be useful for replacing runtime settings type checking
with compile time checking.
-BEGIN VERIFY SCRIPT-
git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
|
|
|
|
|
|
|
|
The m_ prefix indicates that a variable is a data member. Using it as
a parameter name is misleading.
Also update the name of the function from copyStats to CopyStats to
comply with our style guide.
|
|
This allows us to make it const.
|
|
fa9eade142964d5482fe8d2109fb67a9556ae93d Remove GetAddrName (MarcoFalke)
fa786570a5fdf6723b35883054f9f840a3440f92 Remove unused RecursiveMutex cs_addrName (MarcoFalke)
fa82f4ea96749115311cffa0919d49d383c4d28b Remove unused MaybeSetAddrName (MarcoFalke)
Pull request description:
.
ACKs for top commit:
jnewbery:
Code review ACK fa9eade142964d5482fe8d2109fb67a9556ae93d
naumenkogs:
utACK fa9eade142964d5482fe8d2109fb67a9556ae93d
Tree-SHA512: 61501a699add59225dc8127b6dfdda450d768c86f958fdf94e9c28309c3705ecfbee4b064d44228b8c1190c19c39272becc7ede8386ac1406699ea2285881c72
|
|
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#c131-avoid-trivial-getters-and-setters
|
|
fa2547fc52b90b4bbde250803df24d7f665383a7 fuzz: Avoid timeout in blockfilter fuzz target (MarcoFalke)
Pull request description:
Previously it would take 10 seconds to run this input, now it takes 10ms: [clusterfuzz-testcase-blockfilter-5022838196142080.log](https://github.com/bitcoin/bitcoin/files/7021883/clusterfuzz-testcase-blockfilter-5022838196142080.log)
The fix is moving the `MatchAny` out of the hot loop.
Also, to avoid unlimited runtime, cap the hot loop at 30k iterations.
ACKs for top commit:
GeneFerneau:
Approach ACK [fa2547f](https://github.com/bitcoin/bitcoin/pull/22755/commits/fa2547fc52b90b4bbde250803df24d7f665383a7)
Tree-SHA512: a04e7388856930ec81222da8f05b665a923fe9482aeb4c55c9be4561aa7320a0703dbbf8d438ae92854e877a8e3b46777a29c0b652b8f34c29c2142cc5d63ccb
|
|
CAddrMan::m_asmap is now set directly in AppInitMain() so
CConnMan::SetAsmap() is no longer required.
|
|
This logic is a no-op since it was introduced in commit
f9f5cfc50637f2cd1540923caf337e2651ec1625.
m_addr_name is never initialized to the empty string, because
ToStringIPPort never returns an empty string.
|
|
std::optional<CAmount>
f7752adba5dd35fccd3f2144cfcf03538ebf275b util: check MoneyRange() inside ParseMoney() (fanquake)
5ef2738089efd396186775ad23aaec71ea44ebb1 util: make ParseMoney return a std::optional<CAmount> (fanquake)
Pull request description:
Related discussion in #22193.
ACKs for top commit:
MarcoFalke:
review ACK f7752adba5dd35fccd3f2144cfcf03538ebf275b 📄
Tree-SHA512: 88453f9e28f668deff4290d4bc0b2468cbd54699a3be1bfeac63a512276d309354672e7ea7deefa01466c3d9d826e837cc1ea244d4d74b4fa9c11c56f074e098
|
|
|
|
|
|
Also, access mapRecvBytesPerMsgCmd with `at()` not `find()`. This
throws an error if COMMAND_OTHER doesn't exist, which should never
happen. `find()` instead just accessed the last element, which could make
debugging more difficult.
Resolves review comments from PR19107:
- https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478718436
- https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478714497
|
|
Just use unique_ptr<CAddrMan>s and reset the pointer if a frest addrman is required.
Also make CAddrMan::Clear() private to ensure that no call sites are missed.
|
|
60e0cbdd574bb9109bcad1e0c27c7936a534a0e7 [addrman] Merge the two Add() functions (Amiti Uttarwar)
Pull request description:
This PR merges the two definitions of this overloaded function to reduce code duplication.
When these functions were introduced in https://github.com/bitcoin/bitcoin/commit/5fee401fe14aa6459428a26a82f764db70a6a0b9, there were multiple places that invoked `Add()` with a single addr and a vector of addrs each, so it made sense to overload the function. I could see how the small difference in log statement was more meaningful when a peer was added via IRC :)
Now, the definition of `Add()` that takes in a single address is only invoked from the hidden/test-only RPC `addpeeraddress`. These changes should not cause any observable difference, and are covered by the existing tests that use this RPC endpoint.
ACKs for top commit:
jnewbery:
Code review ACK 60e0cbdd574bb9109bcad1e0c27c7936a534a0e7
Zero-1729:
crACK 60e0cbd
fanquake:
ACK 60e0cbdd574bb9109bcad1e0c27c7936a534a0e7
Tree-SHA512: 782fb2ac6d2d403ba7d7ff543197ca42b610b9a8806952d271e57e2ee3527ad1a94af4ebbad5371b5e95d77df07c56ccc8c1d5a2c82cdecb0d2b5085b3bdd5ee
|
|
fa7718344d2879bb3f3c00a4185c5445390c017d fuzz: Avoid OOM in system fuzz target (MarcoFalke)
Pull request description:
If the inputs size is unlimited, the target may consume unlimited memory, because the argsmanager stores the argument names. Limiting the size should fix this issue.
Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=36906
ACKs for top commit:
practicalswift:
cr ACK fa7718344d2879bb3f3c00a4185c5445390c017d
Tree-SHA512: 6edfcf324ee9d94e511038ee01340f02db50bcb233af3f1a1717c3602164c88528d9d987e971ec32f1a4593b868019bea0102c53c9b02bfefec3dfde959483cf
|
|
Merge the two definitions of this overloaded function to reduce code
duplication.
|
|
a4d78546b0858602c60c03fdf8b35ca666ab2e56 [addrman] Make addrman consistency checks a runtime option (John Newbery)
10aac241455a3270462d49b53732477ed97623e7 [tests] Make deterministic addrman use nKey = 1 (John Newbery)
fa9710f62c29c7f8d71c9f281001c9b5e70946bf [addrman] Add deterministic argument to CAddrMan ctor (John Newbery)
ee458d84fc187d69f002ebead6fccc4f4f9c0744 Add missing const to CAddrMan::Check_() (MarcoFalke)
Pull request description:
CAddrMan has internal consistency checks. Currently, these are only run when the program is compiled with the `DEBUG_ADDRMAN` option. This option is not enabled on any of our CI builds, and it's likely that no-one is running them at all.
This PR makes consistency checks a (hidden) runtime option that can be enabled with `-checkaddrman`, where `-checkaddrman=n` will result in the consistency checks running every n operations (similar to `-checkmempool=n`). We set the ratio to 1/100 for our unit tests, and leave it disabled by default for all networks. Additionally, a consistency check failure now asserts, rather than logging and continuing. This matches the behavior of CTxMemPool and TxRequestTracker, where a failed consistency check asserts.
ACKs for top commit:
jonatack:
ACK a4d78546b0858602c60c03fdf8b35ca666ab2e56 per `git diff 00fd089 a4d7854`, tested by adding logging similar to #22479 and running with `-checkaddrman=<n>` for various values 0/1/10/100 etc, tested the updated docs with `bitcoind -help-debug | grep -A2 "checkaddrman\|checkmempool"` and verified rebased on master that compiling with `CPPFLAGS="-DDEBUG_ADDRMAN"` no longer causes the build to error.
mzumsande:
Code-review ACK a4d78546b0858602c60c03fdf8b35ca666ab2e56
theStack:
Code-review ACK a4d78546b0858602c60c03fdf8b35ca666ab2e56
Tree-SHA512: eaee003f7a99154822c5b5efbc62008d32c1efbecc6fec6e183427f6b2ae5d30b3be7924e3a7271b1a1de91517f5bd2a70011d45358c3105c6a0702f12b70f7c
|
|
Currently addrman consistency checks are a compile time option, and are not
enabled in our CI. It's unlikely anyone is running these consistency checks.
Make them a runtime option instead, where users can enable addrman
consistency checks every n operations (similar to mempool tests). Update
the addrman unit tests to do internal consistency checks every 100
operations (checking on every operations causes the test runtime to
increase by several seconds).
Also assert on a failed addrman consistency check to terminate program
execution.
|
|
92993aa5cf37995e65e68dfd6f129ecaf418e01c Change SignTransaction's input_errors to use bilingual_str (Andrew Chow)
171366e89b828a557f8262d9dc14ff7a03f813f7 Use bilingual_str for address fetching functions (Andrew Chow)
9571c69b51115454c6a699be9492024f7b46c2b4 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 92993aa5cf37995e65e68dfd6f129ecaf418e01c, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22337#pullrequestreview-694542729) review, verified with
klementtan:
Code review ACK 92993aa5cf37995e65e68dfd6f129ecaf418e01c
meshcollider:
Code review ACK 92993aa5cf37995e65e68dfd6f129ecaf418e01c
Tree-SHA512: 5400e419dd87db8c49b67ed0964de2d44b58010a566ca246f2f0760ed9ef6a9b6f6df7a6adcb211b315b74c727bfe8c7d07eb5690b5922fa5828ceef4c83461f
|
|
|
|
|
|
Removes the need for tests to update nKey and insecure_rand after constructing
a CAddrMan.
|
|
AddrMan
87651795d8622d354f8e3c481eb868d9433b841c fuzz: check that ser+unser produces the same AddrMan (Vasil Dimov)
6408b24517f3418e2a408071b4c2ce26571f3167 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 87651795d8622d354f8e3c481eb868d9433b841c
jonatack:
ACK 87651795d8622d354f8e3c481eb868d9433b841c rebased to current master, reviewed, fuzz build, ran `FUZZ=addrman_serdeser src/test/fuzz/fuzz`
Tree-SHA512: 7eda79279f14f2649840bf752e575d7b02cbaad541f74f7254855ebd4a32da988f042d78aa9228983350283bb74dd0c71f51f04c0846889c3ba2f19f01a0c303
|
|
|
|
|
|
|
|
Move the addrman init code from the test case to a newly added
`CAddrManDeterministic` constructor. This way it can be reused by other
tests.
|
|
fa1eddb1a3d1319ddc3643b6f34fe2014de32764 Fix whitespace in touched files (MarcoFalke)
fa4e6afdae7b82df638b60edf37ac36d57a8cb4f Remove unused CSubNet serialize code (MarcoFalke)
fa384fdd0b7af73d81fa9619c5fba779452cd2af 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 fa1eddb1a3d1319ddc3643b6f34fe2014de32764
laanwj:
concept and code review ACK fa1eddb1a3d1319ddc3643b6f34fe2014de32764
vasild:
ACK fa1eddb1a3d1319ddc3643b6f34fe2014de32764
jonatack:
Light code review utACK fa1eddb1a3d1319ddc3643b6f34fe2014de32764
Tree-SHA512: e136193b7c0ba1d6d2e79c7fb4106ba4af75fa229ed7214675ee64e98e59bb4808779e7a8a09eecce62f7a5d4bc6e16b8a5ad4596129357c8fc5e3b88f214249
|
|
fae108ceb53f61d7338ba205873623ede3c1d3be Fix incorrect whitespace in addrman (MarcoFalke)
fa32024d51c098441623e246f304a80f011e29d1 Add missing GUARDED_BY to CAddrMan::insecure_rand (MarcoFalke)
fab755b77f88873f01cbd988051de7ad3f0150de fuzz: Actually use const addrman (MarcoFalke)
fae0c79351ce34186249d44af0c5c9c7521f4b6c refactor: Mark CAddrMan::GetAddr const (MarcoFalke)
fa02934c8c9d290ea4d12683e8680c70967a4d3a 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 fae108ceb53f61d7338ba205873623ede3c1d3be
theStack:
re-ACK fae108ceb53f61d7338ba205873623ede3c1d3be 🍦
Tree-SHA512: 3ffb211d4715cc3daeb3bfcdb3fcc6b108ca96df5fa565510436fac0e8da86c93b30c9c4aad0563e27d84f615fcd729481072009a4e2360c8b3d40787ab6506a
|