c62d763fc313585d79ad833c9d729f6acf2652aa Necessary improvements to make configure work without libevent installed (Perlover)
091ccc38c2e589b649648cbcc99aca4802f98775 The evhttp_connection_get_peer function from libevent changes the type of the second parameter. Fixing the problem. (Perlover)
Pull request description:
The second parameter of evhttp_connection_get_peer in libevent already has type as `const char **`
The compilation of bitcoind with the fresh libevent occurs errors
Details: https://github.com/bitcoin/bitcoin/issues/23606
ACKs for top commit:
laanwj:
Code review ACK c62d763fc313585d79ad833c9d729f6acf2652aa
luke-jr:
tACK c62d763fc313585d79ad833c9d729f6acf2652aa
Tree-SHA512: d1c8062d90bd0d55c582dae2c3a7e5ee1b6c7ca872bf4aa7fe6f45a52ac4a8f59464215759d961f8efde0efbeeade31b08daf9387d7d50d7622baa1c06992d83
b5c9bb5cb9f4a8db57b33ef7399310c7d6de5822 util: Restore GetIntArg saturating behavior (James O'Beirne)
Pull request description:
The new locale-independent atoi64 method introduced in #20452 parses large integer values higher than maximum representable value as 0 instead of the maximum value, which breaks backwards compatibility. This commit restores compatibility and adds test coverage for this case in terms of the related GetIntArg and strtoll functions.
Specifically, command line or bitcoin.conf integer values greater than `9223372036854775807` (`2**63-1`) used to be parsed as `9223372036854775807` before #20452. Then #20452 caused them to be parsed as `0`. And after this PR they will be parsed as `9223372036854775807` again.
This change is a stripped-down alternative version of #23841 by jamesob
ACKs for top commit:
jamesob:
Github ACK b5c9bb5cb9
vincenzopalazzo:
ACK b5c9bb5cb9
MarcoFalke:
review ACK b5c9bb5cb9f4a8db57b33ef7399310c7d6de5822 🌘
Tree-SHA512: 4e8abdbabf3cf4713cf5a7c5169539159f359ab4109a4e7e644cc2e9b2b0c3c532fad9f6b772daf015e1c5340ce59280cd9a41f2730afda6099cbf636b7d23ae
3c4f5d2a2024a71523a2e43e8144cdb0818dee6f scripted-diff: Rename functions to drop mention of boost (Hennadii Stepanov)
Pull request description:
Nothing specific to Boost is in `GUIUtil::qstringToBoostPath` and `GUIUtil::boostPathToQString` functions.
Related to bitcoin/bitcoin#20744.
ACKs for top commit:
laanwj:
Concept and code review ACK 3c4f5d2a2024a71523a2e43e8144cdb0818dee6f
promag:
Code review ACK 3c4f5d2a2024a71523a2e43e8144cdb0818dee6f 👋
Tree-SHA512: 0c8790979783a067811f7699b4ce4c204f6e0818d5f32986ec24b2c71583b4496d7a0e0c0361dd77c7e641a75d983fee35cd51ef722bbca9a5f13194efb3b4c0
The new locale-independent atoi64 method introduced in #20452 parses
large integer values higher than maximum representable value as 0
instead of the maximum value, which breaks backwards compatibility.
This commit restores compatibility and adds test coverage for this case
in terms of the related GetIntArg and strtoll functions.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
BlockManager is a large data structure, and cs_main is not required to
take its address or access every part of it. Individual BlockManager
fields and methods which do require cs_main like m_block_index and
LookupBlockIndex are already annotated separately, and these other
annotations describe locking requirements more accurately and do a
better job enforcing thread safety.
Since cs_main is not needed to access the address of the m_block object,
this commit drops cs_main LOCK calls which were added pointlessly to
satisfy this annotation in the past.
Co-authored-by: Carl Dong <contact@carldong.me>
e5b6aef61221b621ad77b5f075a16897e08835bf Move CBlockFileInfo::ToString method where class is declared (Russell Yanofsky)
f7086fd8ff084ab0dd656d75b7485e59263bdfd8 Add src/wallet/* code to wallet:: namespace (Russell Yanofsky)
90fc8b089d591cabff60ee829a33f96c37fd27ba Add src/node/* code to node:: namespace (Russell Yanofsky)
Pull request description:
There are no code changes, this is just adding `namespace` and `using` declarations and `node::` or `wallet::` qualifiers in some places.
Motivations for this change are:
- To make it easier to see when node and wallet code is being accessed places where it shouldn't be. For example if GUI code is accessing node and wallet internals or if wallet and node code are referencing each other.
- To make source code organization clearer ([#15732](https://github.com/bitcoin/bitcoin/issues/15732)), being able to know that `wallet::` code is in `src/wallet/`, `node::` code is in `src/node/`, `init::` code is in `src/init/`, `util::` code is in `src/util/`, etc.
Reviewing with `git log -p -n1 -U0 --word-diff-regex=.` can be helpful to verify this is only updating declarations, not changing code.
ACKs for top commit:
achow101:
ACK e5b6aef61221b621ad77b5f075a16897e08835bf
MarcoFalke:
Concept ACK e5b6aef61221b621ad77b5f075a16897e08835bf 🍨
Tree-SHA512: 3797745c90246794e2d55a2ee6e8b0ad5c811e4e03a242d3fdfeb68032f8787f0d48ed4097f6b7730f540220c0af99ef423cd9dbe7f76b2ec12e769a757a2c8d
3a45dc36a663ea67f13e7d5c413518b9385ec594 Change type of `backup_file` parameter in RestoreWallet/restoreWallet (Hennadii Stepanov)
213172c7348091794b173c2edf5f7f3bbe010912 refactor: Block unsafe std::string fs::path conversion copy_file calls (Hennadii Stepanov)
Pull request description:
This PR is an optional prerequisite for bitcoin/bitcoin#20744 "Use std::filesystem. Remove Boost Filesystem & System" which:
- makes further code changes safer
- prevents [some](https://cirrus-ci.com/task/6525835388649472) test failures on native Windows
ACKs for top commit:
ryanofsky:
Code review ACK 3a45dc36a663ea67f13e7d5c413518b9385ec594. Looks great! Thanks for debugging and fixing this and making #20744 smaller!
Tree-SHA512: c6dfaef6b45b9c268bc9ee9b943b9d679152c9d565ca4f86da8d33f8eb9b3cdbe9ba6df7b7578eacc0d00db6551048beff97419f86eb4b1d3182c43e2b4eb9a5
`fs::path` looks more native than `std::string` for a parameter which
represents a backup file. This change eliminates back-and-forth type
conversions.
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding
copy_file calls that will be unsafe after the transition to
std::filesystem to due lack of a boost::filesystem::path::imbue
equivalent and inability to set a predictable locale.
fa8e01a5b8c67fcb8acaaf58a17ed24710415b6c doc: Remove outdated scriptChange TODO comment (MarcoFalke)
Pull request description:
This was added in commit bf798734db4539a39edd6badf54a1c3aecf193e5 (raw multisig). Raw multisig isn't a thing, so remove the TODO.
ACKs for top commit:
S3RK:
ACK fa8e01a5b8c67fcb8acaaf58a17ed24710415b6c
achow101:
ACK fa8e01a5b8c67fcb8acaaf58a17ed24710415b6c
Tree-SHA512: 01d521ca3605ab130c43531da4922ea85461ca5e7436267a34fb5df348009e086b3c66d85532c62255d9a0ba43db56424884808e773d0ef0177035dfb25d6a6c
efab28b06bfaa50c41337e84136cb58437e7ba00 Add FastRange32 function and use it throughout the codebase (Pieter Wuille)
96ecd6fa3e0f53c3a25eb7c328220b819f8dde03 scripted-diff: rename MapIntoRange to FastRange64 (Pieter Wuille)
c6d15c45d971fb25551b7a66a2615e3f0bee999b [moveonly] Move MapIntoRange() to separate util/fastrange.h (Pieter Wuille)
Pull request description:
Several places in the codebase use the fast range mapping technique described in https://lemire.me/blog/2016/06/27/a-fast-alternative-to-the-modulo-reduction/, some for 32-bit ranges, some for 64-bit ones.
Move all of these to `util/fastrange.h`, and give them a consistent name.
ACKs for top commit:
Sjors:
ACK efab28b06bfaa50c41337e84136cb58437e7ba00
shaavan:
reACK efab28b06bfaa50c41337e84136cb58437e7ba00
MarcoFalke:
review ACK efab28b06bfaa50c41337e84136cb58437e7ba00 🍸
Tree-SHA512: 3190a25ef21d17f0ab2afcd9b8d5a1813fdfac0d93996878017e84ff62eee412c823d6149ae8e92cfc3214458641e83ace4b022b4a0fe0679f78dbaee21c6227
742918c8ef353993a07c060f476a160e8272a9ef qt: hide Create Unsigned button behind an expert mode option (Andrew Chow)
5c3b800acd3ceb75ff6bbac8d0e2e1aaa95b0728 qt: Add Create Unsigned button to SendConfirmationDialog (Andrew Chow)
Pull request description:
Instead of having different buttons or changing button behavior for making a PSBT, just have SendConfirmationDialog return whether the user wants a PSBT or a broadcasted transaction. Since this dialog is used by both the bumpFeeAction and the SendCoinsDialog, changes to both to support the different behavior is needed. They will check the return value of the SendConfirmationDialog for whether a PSBT needs to be created instead of checking whether private keys are disabled.
Strings used in this dialog are being slightly modified to work with both private keys enabled and disabled wallets.
Moved from https://github.com/bitcoin/bitcoin/pull/18789
ACKs for top commit:
jarolrod:
ACK 742918c
ryanofsky:
Code review ACK 742918c8ef353993a07c060f476a160e8272a9ef. Just suggested changes since last review. Looks great!
hebasto:
ACK 742918c8ef353993a07c060f476a160e8272a9ef, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: dd29f4364c7b4f15befe8fe63257b26187918786b005e0f8336183270b1a162680b93f6ced60f0285c6e607c084cc0d24950fc68a8f9c056521ede614041be66
The new helper function, BlockManager::WriteBlockIndexDB(),
has a thread safety lock annotation in its declaration but is
missing the corresponding run-time lock assertion in its definition.
Per doc/developer-notes.md: "Combine annotations in function
declarations with run-time asserts in function definitions."
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
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
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
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
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
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
