fa68a6c2fc6754c160e0f98007785602201b3c47 scripted-diff: Rename touched member variables (MarcoFalke)
facd3df21f344dd84e5f28862056700c1fded17c Make blockstorage globals private members of BlockManager (MarcoFalke)
faa8c2d7d75f8d9782709e73e00e35851e233392 doc: Clarify nPruneAfterHeight for signet (MarcoFalke)
fad381b2f8e1beb18f748fbeb820e63545b9b0fd test: Load genesis block to allow flush (MarcoFalke)
fab262174b96854d2df5bee7da578990c9e9cb1e Move blockstorage-related unload to BlockManager::Unload (MarcoFalke)
fa467f3913918701c765f9bc754203b4591b894f move-only: Create WriteBlockIndexDB helper (MarcoFalke)
fa88cfd3f9896d5b56ea6c111a23f90a79253c18 Move functions to BlockManager (MarcoFalke)
Pull request description:
Globals aren't too nice because they hide dependencies, also they make testing harder.
Fix that by removing some.
ACKs for top commit:
Sjors:
ACK fa68a6c2fc6754c160e0f98007785602201b3c47
ryanofsky:
Code review ACK fa68a6c2fc6754c160e0f98007785602201b3c47. Nice changes!
Tree-SHA512: 6abc5929a5e43a05e238276721d46a64a44f23dca18c2caa9775437a32351d6815d88b88757254686421531d0df13861bbd3a202e13a3192798d87a96abef65d
c9374af10227be3e4c9d4fb5fbd1027841361f14 test: set ban after mocking time (brunoerg)
Pull request description:
Fixes #23988
Set ban after mocking time to avoid intermittent failures related to the assertion of ban_duration and time_remaining.
See: https://cirrus-ci.com/task/6754020390862848?logs=ci#L4652
ACKs for top commit:
mzumsande:
Tested ACK c9374af10227be3e4c9d4fb5fbd1027841361f14
vincenzopalazzo:
ACK c9374af102
Tree-SHA512: fac3ac91a045bb46334d7c568f6a53a3b0a45b306914a54ea13bcc845734eaaad1ff295ff3ab158037fd9d08df77344058331336110b8f7888832b16b0589be5
0754e9c01bd3d57aa241e313ba34c18c4897ba98 test: run feature_pruning.py without wallet compiled (Sebastian Falbesoner)
Pull request description:
Only one small part of the pruning test (sub-test `wallet_test`) is wallet-related, hence we can run all other parts without wallet compiled.
ACKs for top commit:
MarcoFalke:
cr ACK 0754e9c01bd3d57aa241e313ba34c18c4897ba98
Tree-SHA512: 856856903d21d64953ed0102cc2a96f55975c4b7d8e93e57b82c266024967160df64df2b6068be089efc05e883e8d6d12e7327053420d4c640b9d8cc5bcb1c58
172096e9dd5b0e04b88ffd6dc1f6d43190070e14 scripted-diff: Rename libbitcoin_server.a to libbitcoin_node.a (Russell Yanofsky)
Pull request description:
Goal along with namespacing PR #23497 is to make code organization more obvious and have `src/node/` code in `node::` namespace in `libbitcoin_node.a` library
ACKs for top commit:
MarcoFalke:
cr ACK 172096e9dd5b0e04b88ffd6dc1f6d43190070e14
Tree-SHA512: a2e787eeaa3ab769b0f5376473072cae584d237aa8b67b677bea833bb36b0134a0eca17eb01722389639473b8463f4953bc3a5e4801a6b2c8965ac1075cba005
e3544c864e3e56867de25b8db7b012d58b378050 init: Use clang-tidy named args syntax (Carl Dong)
3401630417d994b53ff3a89db2ea759ab1ec6f0f style-only: Rename *Chainstate return values (Carl Dong)
1dd582782d3c182aa952f23ec577f6a0a8672e7b docs: Make LoadChainstate comment more accurate (Carl Dong)
6b83576388e205116a0ebc67b9949f309eea1207 node/chainstate: Use MAX_FUTURE_BLOCK_TIME (Carl Dong)
Pull request description:
There are 2 proposed fixups in discussions in #23280 which I have not implemented:
1. An overhaul to return types and an option type for the two `*Chainstate` functions: https://github.com/bitcoin/bitcoin/pull/23280#issuecomment-984149564
- The change reintroduces stringy return types and is quite involved. It could be discussed in a separate PR.
2. Passing in the unix time to `VerifyChainstate` instead of a callback to get the time: https://github.com/bitcoin/bitcoin/pull/23280#discussion_r765051533
- I'm not sure it matters much whether it's a callback or just the actual unix time. Also, I think `VerifyDB` can take quite a while, and I don't want to impose that the function have to "run quickly" in order to have it be correct.
If reviewers feel strongly about either of the two fixups listed above, please feel free to open a PR based on mine and I'll close this one!
ACKs for top commit:
ryanofsky:
Code review ACK e3544c864e3e56867de25b8db7b012d58b378050
MarcoFalke:
ACK e3544c864e3e56867de25b8db7b012d58b378050 🐸
Tree-SHA512: dd1de0265b6785eef306e724b678ce03d7c54ea9f4b5ea0ccd7af59cce2ea3aba73fd4af0c15e2dca9265807dc4075f9afa2ec103672677b6638b1a4fc090904
fe86eb50c986f7b5ccce63f984d8a51cd9ee9e2c Refactor: Uses c++ init convention for time variables (Shashwat)
6111b0d7fac89b7a0a03284ca6ec030ca7f30b99 Refactor: Changes remaining time variable type from int to chrono (Shashwat)
Pull request description:
This PR is a follow-up to #23801.
This PR aims to make the following changes to all the time variables in **net_processing.cpp** wherever possible.
- Convert all time variables to `std::chrono.`
- Use `chorno::literals` wherever possible.
- Use `auto` keywords wherever possible.
- Use `var{val}` convention of initialization.
This PR also minimizes the number of times, serialization of time `count_seconds(..)` occurs.
ACKs for top commit:
MarcoFalke:
re-ACK fe86eb50c986f7b5ccce63f984d8a51cd9ee9e2c 🏕
Tree-SHA512: c8684c0c60a11140027e36b6e9706a45ecdeae6b5ba0bf267e50655835daee5e5410e34096a8c4eca005f327caae1ac258cc7b8ba663eab58abf131f6d2f4d42
60ae1161a4c415cc73f47df95598f3688e8d34df qa: replace assert with test framework assertion helpers in fee estimation test (Antoine Poinsot)
e50213967b6d5dda9c0acc4643c8ec67f9fd7284 qa: fee estimation with RBF test cleanups (Antoine Poinsot)
15f5fd62afb57ec501dc8c6706999d4c83e58414 qa: don't mine non standard txs in fee estimation test (Antoine Poinsot)
eae52dd6abb8efb99d62d38670cea89ff1e41286 qa: pass scriptsig directly to txins constructor in fee estimation test (Antoine Poinsot)
1fc03155e53f4f0a7f0a2529e55e802f49260b23 qa: split coins in a single tx in fee estimation test (Antoine Poinsot)
cc204b8be758102bba94e8e3e0d1989462cb9e5c qa: use a single p2sh script in fee estimation test (Antoine Poinsot)
19dd91a9bec77b14ff5b883d3e185b6b4a081ef7 qa: remove a redundant condition in fee estimation test (Antoine Poinsot)
Pull request description:
Some cleanups that i noticed would be desirable while working on #23074 and #22539, which are intentionally not based on it. Mainly simplifications and a slight speedup.
- Use a single tx to create the `2**9` coins instead of creating `2**8` 2-outputs transactions
- Use a single P2SH script
- Avoid the use of non-standard transactions
- Misc style nits (happy to take more)
ACKs for top commit:
pg156:
I ACK all commits up to 60ae1161a4 (except 1fc03155e5, where I have a question more for my own learning than actually questioning the PR). I built and ran the test successfully. I agree after the changes, the behavior is kept the same and the code is shorter and easier to reason.
glozow:
utACK 60ae1161a4c415cc73f47df95598f3688e8d34df
Tree-SHA512: 57ae2294eb68961ced30f32448c4a530ba1cdee17881594eecb97e1b9ba8927d58c25022b847eb07fb67d676bf436108c416c2f2174864d258fcca5b528b8bbd
eBPF is a Linux kernel technology used to "extend the capabilities
of the kernel without requiring to change kernel source code or
load kernel modules". While Userspace, Statically Defined Tracing
(USDT) uses eBPF under the hood, --enable-usdt better resembles that
support for USDT is enabled, and tracepoints will be included in the
binary.
9bd46e24a67e000fdbd76dffde6da25c12e29dac reviewers: add tracing (William Casarin)
Pull request description:
Adding myself and 0xB10C to USDT tracing reviews
~~cc 0xB10C lmk if you want in on this~~
ACKs for top commit:
0xB10C:
ACK 9bd46e24a67e000fdbd76dffde6da25c12e29dac
Tree-SHA512: 8c6674bcf0fc70422163d353ad22d4bb3d747af9d17de069d39b4576ec3e1baf883605774683f9caf84d138ae1dc1424d1a9afdd115abba9d947cbc31a309144
fa9f4554ca5e64e7960b74dd0ad8fb1cd7c2f091 refactor: Remove pointless and confusing shift in RelayAddress (MarcoFalke)
Pull request description:
The second argument written to the siphash is already quantized to 24 hours, so it seems confusing to quantize the first argument to 32 bits (out of 64 bits).
> The shifting is pointless, we should get rid of it. It seems to be a silly evolution of this 2010 Satoshi code: 5cbf753 (where it made sense because everything was XORed together, and the address used the high bits, while the time used the low ones).
(Copied from https://github.com/bitcoin/bitcoin/pull/18642#issuecomment-613773120)
(The original code was `uint256 hashRand = hashSalt ^ (((int64)addr.ip)<<32) ^ ((GetTime()+addr.ip)/(24*60*60));`)
This also allows to remove a integer sanitizer suppression for the whole file.
ACKs for top commit:
laanwj:
Code review ACK fa9f4554ca5e64e7960b74dd0ad8fb1cd7c2f091
sipa:
utACK fa9f4554ca5e64e7960b74dd0ad8fb1cd7c2f091
promag:
Code review ACK fa9f4554ca5e64e7960b74dd0ad8fb1cd7c2f091.
Tree-SHA512: f5fd107464ccd839d6749aed6914b4935e39ab42906546b3f3810a7339fc4633fef931a1783a287572af5ec64525626fa91d147d8ff52eb076740465bf5cf839
ac617cc141fe05bea0dc5e8f9df3da43c0945842 wallettool: Check that the dumpfile checksum is the correct size (Andrew Chow)
Pull request description:
After parsing the checksum, make sure that it is the size that we expect it to be.
This issue was reported by Pedro Baptista.
ACKs for top commit:
laanwj:
Code review ACK ac617cc141fe05bea0dc5e8f9df3da43c0945842
Tree-SHA512: 8135b3fb1f4f6b6c91cfbac7d1d3421f1f6c664a742c92940f68eae857f92ce49d042cc3aa5c2df6ef182825271483d65efc7543ec7a8ff047fd7c08666c8899
8bd34dc774788cbf3cad8e139542a0ed9f3e8bb4 test: check that bitcoin-tx detects missing input amount for segwit transactions (Sebastian Falbesoner)
c337b27d7cfd468048bcf816e585a1f7d59e066d Require that input amount is provided for bitcoin-tx witness transactions (Ben Woosley)
Pull request description:
This PR picks up the obviously abandoned PR #13608 (last activity was three and a half years ago) by rebasing it on master and adding missing tests. Original PR description: "_Applies fix from #12458 / #13547 to bitcoin-tx._"
The private key is the compressed version of the one used in most other util tests (5HpHagT65TZzG1PH3CSu63k8DbpvD8s5ip4nEB3kEsreAnchuDf, corresponds to the scalar value k=1 in big endian), since segwit signing refuses uncompressed keys.
The error message from the picked up PR is changed to not include the amount, as showing any value would be just confusing.
ACKs for top commit:
josibake:
ACK 8bd34dc774
Tree-SHA512: 334b418f89527363ad7e3326b4126e86a05fd64876c49a8280de38e64cfac52cb62c4b24b83603dd68b6bcebbe57c64161832edffb1cac7e9c68426f6b6eae1f
975097f424541a149c5b4e03816208aa76aad6b9 Let test_runner.py start multiple jobs per timeslot (Pieter Wuille)
Pull request description:
test_runner.py currently only checks every 0.5s whether any job has finished, and if so, starts at most one new job. At higher parallellism it becomes increasingly likely that multiple jobs have finished at the same time. Fix this by always noticing *all* finished jobs every timeslot, and starting as many new ones.
ACKs for top commit:
laanwj:
Code review and lightly tested ACK 975097f424541a149c5b4e03816208aa76aad6b9
prayank23:
ACK 975097f424
Tree-SHA512: b70c51f05efcde9bc25475c192b86e86b4c399495b42dee20576af3e6b99e8298be8b9e82146abdabbaedb24a13ee158a7c8947518b16fc4f33a3b434935b550
This is needed to turn globals into member variables. Otherwise, this
will lead to issues:
runtime error: reference binding to null pointer of type 'CBlockFileInfo'
#0 in std::vector<CBlockFileInfo, std::allocator<CBlockFileInfo> >::operator[](unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:1046:2
#1 in BlockManager::FlushBlockFile(bool, bool) src/node/blockstorage.cpp:540:47
#2 in CChainState::FlushStateToDisk(BlockValidationState&, FlushStateMode, int) src/validation.cpp:2262:28
#3 in CChainState::ResizeCoinsCaches(unsigned long, unsigned long) src/validation.cpp:4414:15
#4 in validation_chainstate_tests::validation_chainstate_resize_caches::test_method() src/test/validation_chainstate_tests.cpp:66:12
This is a refactor and safe to do because:
* UnloadBlockIndex calls ChainstateManager::Unload, which calls
BlockManager::Unload
* Only unit tests call Unload directly
fab16415ba1729748b03518c92396febda5a25d1 doc: Fix typo in getmempoolinfo (MarcoFalke)
Pull request description:
Also, remove whitespace. Can be reviewed with `--ignore-all-space --word-diff-regex=.`.
ACKs for top commit:
laanwj:
Good catch. ACK fab16415ba1729748b03518c92396febda5a25d1
shaavan:
ACK fab16415ba1729748b03518c92396febda5a25d1
Tree-SHA512: 9d51ef4a4eccfcf437c99a9f84f48e4f090d75715332ad2b4cf10ad77c3691de03255b4817e9fd203fad9baf338066982304dc62fab93dd605e735943f8ca346
6bf6e9fd9dece67878595a5f3361851c25833c49 net: change CreateNodeFromAcceptedSocket() to take Sock (Vasil Dimov)
9e3cbfca7c9efa620c0cce73503772805cc1fa82 net: use Sock in CConnman::ListenSocket (Vasil Dimov)
f8bd13f85ae5404adef23a52719d804a5c36b1e8 net: add new method Sock::Accept() that wraps accept() (Vasil Dimov)
Pull request description:
_This is a piece of https://github.com/bitcoin/bitcoin/pull/21878, chopped off to ease review._
Introduce an `accept(2)` wrapper `Sock::Accept()` and extend the usage of `Sock` in `CConnman::ListenSocket` and `CreateNodeFromAcceptedSocket()`.
ACKs for top commit:
laanwj:
Code review ACK 6bf6e9fd9dece67878595a5f3361851c25833c49
jamesob:
ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 ([`jamesob/ackr/21879.2.vasild.wrap_accept_and_extend_u`](https://github.com/jamesob/bitcoin/tree/ackr/21879.2.vasild.wrap_accept_and_extend_u))
jonatack:
ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 per `git range-diff ea989de 976f6e8 6bf6e9f` -- only change since my last review was `s/listen_socket.socket/listen_socket.sock->Get()/` in `src/net.cpp: CConnman::SocketHandlerListening()` -- re-read the code changes, rebase/debug build/ran units following my previous full review (https://github.com/bitcoin/bitcoin/pull/21879#pullrequestreview-761251278)
w0xlt:
tACK 6bf6e9f
Tree-SHA512: dc6d1acc4f255f1f7e8cf6dd74e97975cf3d5959e9fc2e689f74812ac3526d5ee8b6a32eca605925d10a4f7b6ff1ce5e900344311e587d19786b48c54d021b64
c03cf38a16507568da73ce7603cd77cfe1d60392 doc: Fix typo in LoadBlockIndex (brunoerg)
Pull request description:
Instad -> Instead
Top commit has no ACKs.
Tree-SHA512: 37dcdd34e2bd985619daecc9d1072ac002f3bc7b47db413432d027a2b8cce32501f93a57ee85869755bb0eedecedfab6ecac9ce2a591341c8f011118390a5b18
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
7746606cfa31ee7c6d444b313250868ad0e4ac64 test: use MiniWallet for mining_basic.py (Sebastian Falbesoner)
Pull request description:
This PR enables one more of the non-wallet functional tests (mining_basic.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078.
ACKs for top commit:
brunoerg:
crACK 7746606cfa31ee7c6d444b313250868ad0e4ac64
Tree-SHA512: 4455b8b764413b0fc3ef388e3c5d5758f9e6b6d3193ac660269a9ba1c988022e6b7bc148549c2167942ea472c5aaddd2b6b3b9d99790d0654b089af975b86e11
96eb0093d07c58ad3b02c49a5a4385da37a8e337 test: wait rather than assert presence of file in startupnotify test (fanquake)
Pull request description:
Should fix #23967.
ACKs for top commit:
brunoerg:
crACK 96eb0093d07c58ad3b02c49a5a4385da37a8e337
kristapsk:
utACK 96eb0093d07c58ad3b02c49a5a4385da37a8e337
Tree-SHA512: 9107970e45c027cfc6c6cbfcfd5a7d9f5956259bbbb11f5b180c3947126e42e62c0f8ffd69cf7b39b51c9c5b4fedbb753839d59aebe876be68c1484bb6065819
e09773d20a9230ba7aa2cbb7e87fdc5187ddfec6 build: use a static .tiff for macOS .dmg over generating (fanquake)
Pull request description:
For demonstration, after [discussion in #23778](https://github.com/bitcoin/bitcoin/pull/23778#issuecomment-1003005503), and the question as to why we can't just have a `background.tiff` that we copy into the macOS DMG, and do away with the somewhat convoluted image generation steps.
From my understanding, the only reason we have this image generation as part of our build system is so that forks of Core can adapt the imagery for their own branding via `PACKAGE_NAME`. It don't think it provides much value to us, and could just have a static .tiff that we copy into the dmg (replacing the .svg that currently lives in macdeploy/).
Doing this would eliminate the following build dependencies:
For native macOS:
* `sed` (usage in Makefile.am)
* `librsvg` (rsvg-convert)
* `tiffutil`
Linux macOS cross-compile:
* `sed` (usage in Makefille.am)
* `librsvg`
* `tiffcp`
* `convert` (imagemagick)
* `font-tuffy`
Guix Build:
```bash
bash-5.1# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
c98d67796863f4b1bab0ad600d46bd74e744d94072cbd4bc856a6aeaba3bb329 guix-build-e09773d20a92/output/dist-archive/bitcoin-e09773d20a92.tar.gz
3336f90bab312798cb7665e2b4ae24d1a270fb240647d5fed8dbfcd83e3ed37e guix-build-e09773d20a92/output/x86_64-apple-darwin/SHA256SUMS.part
8fd680c7ee158c64bad212385df7b0b302c6c2143d4e672b4b0eb5da41f9256d guix-build-e09773d20a92/output/x86_64-apple-darwin/bitcoin-e09773d20a92-osx-unsigned.dmg
34f54177c2f0700e8cfaf5d85d91e404807cd9d411e22006cdff82653e5f4af2 guix-build-e09773d20a92/output/x86_64-apple-darwin/bitcoin-e09773d20a92-osx-unsigned.tar.gz
da6b8f54ef755d40330c8eac4f5bd0329637e827be9ee61318600d5d0bdcc3dc guix-build-e09773d20a92/output/x86_64-apple-darwin/bitcoin-e09773d20a92-osx64.tar.gz
```
ACKs for top commit:
hebasto:
ACK e09773d20a9230ba7aa2cbb7e87fdc5187ddfec6
jarolrod:
ACK e09773d20a9230ba7aa2cbb7e87fdc5187ddfec6
Zero-1729:
ACK e09773d20a9230ba7aa2cbb7e87fdc5187ddfec6
Tree-SHA512: 0ad06699a5451daa8cfaaa46759eb7bd85254a72e23f857f70d433a2ffb1a4bf6dd464d9c4ac9f8c20aab045f4e2b61c6dcdcbcceef96ce515b1a0c501665b1f
faa51a6aa92966403b1c46d4047f5b3aafcccd1d doc: Mark proprietary array optional (MarcoFalke)
Pull request description:
The global one is returned all the time, but the input/output array is returned optionally
ACKs for top commit:
fanquake:
ACK faa51a6aa92966403b1c46d4047f5b3aafcccd1d
Tree-SHA512: db987c13d59e0ccc633032707438d506fe4f8fbf7569a03b99d899cb1309de94f99c343840107fc51a9f904bcf55e00049808b6cdf732fc16c6e9e818b480936
