| Age | Commit message (Collapse) | Author |
|---|
|
f9ee0f37c28f604bc82dab502ce229c66ef5b3b9 Add comments to CustomUintFormatter (Pieter Wuille)
4eb5643e3538863c9d2ff261f49a9a1b248de243 Convert everything except wallet/qt to new serialization (Pieter Wuille)
2b1f85e8c52c8bc5a17eae4c809eaf61d724af98 Convert blockencodings_tests to new serialization (Pieter Wuille)
73747afbbeb013669faf4c4d2c0903cec4526fb0 Convert merkleblock to new serialization (Pieter Wuille)
d06fedd1bc26bf5bf2b203d4445aeaebccca780e Add SER_READ and SER_WRITE for read/write-dependent statements (Russell Yanofsky)
6f9a1e5ad0a270d3b5a715f3e3ea0911193bf244 Extend CustomUintFormatter to support enums (Russell Yanofsky)
769ee5fa0011ae658770586442715452a656559d Merge BigEndian functionality into CustomUintFormatter (Pieter Wuille)
Pull request description:
The next step of changes from #10785.
This:
* Adds support for enum serialization to `CustomUintFormatter`, used in `CAddress` for service flags.
* Merges `BigEndian` into `CustomUintFormatter`, used in `CNetAddr` for port numbers.
* Converts everything (except wallet and gui) to use the new serialization framework.
ACKs for top commit:
MarcoFalke:
re-ACK f9ee0f37c2, only change is new documentation commit for CustomUintFormatter 📂
ryanofsky:
Code review ACK f9ee0f37c28f604bc82dab502ce229c66ef5b3b9. Just new commit adding comment since last review
jonatack:
Code review re-ACK f9ee0f37c28f604bc82dab502ce229c6 only change since last review is an additional commit adding Doxygen documentation for `CustomUintFormatter`.
Tree-SHA512: e7a0a36afae592d5a4ff8c81ae04d858ac409388e361f2bc197d9a78abca45134218497ab2dfd6d031e0cce0ca586cf857077b7c6ce17fccf67e2d367c1b6cd4
|
|
fabea6d404571d046365f4f083da3569d2cbf4f7 net: Run clang-format on protocol.h (MarcoFalke)
facdeea2b25ef36e37b6ada58ea390a72d11a4b2 net: Remove un-actionable TODO (MarcoFalke)
Pull request description:
The first commit removes a TODO that is infeasible to solve. Currently, most (de)serializable classes in Bitcoin Core have public members. For example `CMessageHeader`, `FlatFilePos`, `CBlock`, `CTransaction`, `CCoin`, ...
So either this TODO comment should apply to all classes or to none. Fix that discrepancy by removing it from the source code for now. If deemed important, the TODO can be discussed in a brainstorming issue later.
Also run clang format on the header file in a new commit. Happy to drop this commit if it is too controversial, but I think it is trivial to review and makes the workflow of developers using clang-format-diff easier.
ACKs for top commit:
practicalswift:
ACK fabea6d404571d046365f4f083da3569d2cbf4f7
naumenkogs:
ACK fabea6d. Not sure why that TODO was there in the first place, but Marco's justification seems correct.
hebasto:
ACK fabea6d404571d046365f4f083da3569d2cbf4f7, agree with both changes: removing TODO and applying the `clang-format-diff.py`.
Tree-SHA512: b79ae07be27e5a40fc9f411a5e9ae91aecb2fdedbcbf74699614a1004f4ef816bf396903ec6c06eb1395fd83a2047620c7583acbaadfb8c4e613319a63062c3c
|
|
faf45d1f1f997c316fc4c611a23c4456533eefe9 http: Avoid crash when g_thread_http was never started (MarcoFalke)
fa12a37b27f0570a551b8c103ea6537ee4a8e399 test: Replace inline-comments with logs, pep8 formatting (MarcoFalke)
fa83b39ff3ae3fbad93df002915c0e5f99c104a9 init: Remove confusing and redundant InitError (MarcoFalke)
Pull request description:
Avoid a crash during shutdown when the init sequence failed for some reason
ACKs for top commit:
promag:
Tested ACK faf45d1f1f997c316fc4c611a23c4456533eefe9.
ryanofsky:
Code review ACK faf45d1f1f997c316fc4c611a23c4456533eefe9. Thanks for updates, this is much easier to parse for me now. Since previous reviews: split out and reverted some cleanups & replaced chmod with mkdir in test
hebasto:
ACK faf45d1f1f997c316fc4c611a23c4456533eefe9, tested on Linux Mint 19.3 with the following patch:
Tree-SHA512: 59632bf01c999e65c724e2728ac103250ccd8b0b16fac19d3a2a82639ab73e4f2efb86c78e63c588a5954625d8d0cf9545e2a7e070e6e15d2a54beeb50e00b61
|
|
WalletModel::pollBalanceChanged
d3a56be77a9d112cde4baef4314882170b9f228f Revert "gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged" (Russell Yanofsky)
bf0a510981ddc28c754881ca21c50ab18e5f2b59 gui: Avoid wallet tryGetBalances calls before TransactionChanged or BlockTip notifications (Russell Yanofsky)
2bc9b92ed8b7736ad67876398a0bb8287f57e9b3 Cancel wallet balance timer when shutdown requested (Russell Yanofsky)
83f69fab3a1ae97c5cff8ba1e6fd191b0fa264bb Switch transaction table to use wallet height not node height (Russell Yanofsky)
Pull request description:
Main commit `gui: Avoid wallet tryGetBalances calls` is one-line change to `WalletModel::pollBalanceChanged` that returns early if there hasn't been a new `TransactionChanged` or `BlockTip` notification since the previous poll call. This is the same behavior that was implemented in #18160, now implemented in a simpler way.
The other commits are a straight revert of #18160, and two tweaks to avoid relying on `WalletModel::m_client_model` lifetime which were causing travis failures with earlier versions of this PR.
Motivation for this change is to be able to revert #18160 and cut down on unnecessary cross-process calls that happen when #18160 is combined with #10102
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
jonasschnelli:
utACK d3a56be77a9d112cde4baef4314882170b9f228f
Tree-SHA512: 3cd31ca515e77c3bd7160d3f1ea0dce5050d4038b2aa441b6f66b8599bd413d81ca5542a197806e773d6092dd1d26830932b1cecbc95298b1f1ab41099e2f12f
|
|
|
|
g_thread_http can not be joined when it is not joinable. Avoid crashing
the node by adding the required check and add a test.
|
|
The "A fatal internal error occurred, see debug.log for details" is
redundant because init.cpp will already show an InitError with a better
error message as well as the hint to check the debug.log
|
|
412d5fe8791c417bf46fc55a5bb8d59be98a33db QA: feature_segwit: Check that template "rules" includes "!segwit" as appropriate (Luke Dashjr)
2abe8cc3b760219cfa434e4c96e9f8d3611d0037 Bugfix: Include "csv","!segwit" in "rules" (Luke Dashjr)
Pull request description:
#16060 removed CSV & segwit from versionbits, breaking the "rules" key returned by GBT.
Without this, miners don't know they're mining segwit blocks, and should fall back to pre-segwit block creation.
ACKs for top commit:
sipa:
ACK 412d5fe8791c417bf46fc55a5bb8d59be98a33db
jnewbery:
Tested ACK 412d5fe8791c417bf46fc55a5bb8d59be98a33db.
Tree-SHA512: 825d72e257dc0dd4941f2fe498d8d4f4f2a21b9505cd21a8f9eb7fb5d6d7dd9219347928cf90bb57a777920ce24295859763e64fa8a22ebb58fc2380f80f5615
|
|
a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc refactor: Remove Node:: queries from GUI (Hennadii Stepanov)
06d519f0b43ed16252428e935d3aeb5a38f582e0 qt: Add SynchronizationState enum to signal parameter (Hennadii Stepanov)
3c709aa69d5bb5a1564c339a0e6a16bac8f02c98 refactor: Remove Node::getReindex() call from GUI (Hennadii Stepanov)
1dab574edf57ccd6cdf5ec706ac328c62142d7a2 refactor: Pass SynchronizationState enum to GUI (Hennadii Stepanov)
2bec309ad6d0f2543948d64ed26f7d9a903f67e5 refactor: Remove unused bool parameter in RPCNotifyBlockChange() (Hennadii Stepanov)
1df77014d8bb733d7d89e36b28671cb47f436292 refactor: Remove unused bool parameter in BlockNotifyGenesisWait() (Hennadii Stepanov)
Pull request description:
This PR is a followup of #18121 and:
- addresses confusion about GUI notification throttling conditions (**luke-jr**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378552386), **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378975960))
- removes `isInitialBlockDownload()` call from the GUI back to the node (on macOS). See: **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#pullrequestreview-357730284)
ACKs for top commit:
jonasschnelli:
Core Review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc (modulo [question](https://github.com/bitcoin/bitcoin/pull/18152#pullrequestreview-414140601)).
ryanofsky:
Code review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc. Only changes since last review were rebase and tweaking SynchronizationState enum declaration as suggested (thanks!)
Tree-SHA512: b6a712a710666e763aeee0d5440de1391a4c6c8f7fa661888773e1ba59e9e0f83654ee384d4edc704031be7eb25616e5eca2a6e26058d3efb7f64c47f9ed7316
|
|
fa243be1dc49385fff847f8a784c7a9c9f07c939 log: Remove "No rpcpassword set" from logs (MarcoFalke)
Pull request description:
rpcpassword is deprecated and not recommended anymore. So remove it from the logs, which indicate that an rpcpassword should be set and cause confusion. See #18998.
ACKs for top commit:
ryanofsky:
Code review ACK fa243be1dc49385fff847f8a784c7a9c9f07c939. New log message makes more sense
elichai:
Re Code Review ACK (Checked the diff) fa243be1dc49385fff847f8a784c7a9c9f07c939
Tree-SHA512: de3e0800a204b15a59a59a7e6f345013ee9d38e8c5d0c9a94d6142780faa9cce672ed358c7571f53c1eb843bf5afb0b7bcbfd289d3b9e2e0bf8ff2fd361e98a9
|
|
2896c412fadbc03916a33028f4f50fd87ac48edb Do not answer GETDATA for to-be-announced tx (Pieter Wuille)
f2f32a3dee9a965c8198f9ddd3aaebc627c273e4 Push down use of cs_main into FindTxForGetData (Pieter Wuille)
c6131bf407c1ada78a0e5509a702bc7da0bfd57d Abstract logic to determine whether to answer tx GETDATA (Pieter Wuille)
Pull request description:
This PR intends to improve transaction-origin privacy.
In general, we should try to not leak information about what transactions we have (recently) learned about before deciding to announce them to our peers. There is a controlled transaction dissemination process that reveals our transactions to peers that has various safeguards for privacy (it's rate-limited, delayed & batched, deterministically sorted, ...), and ideally there is no way to test which transactions we have before that controlled process reveals them. The handling of the `mempool` BIP35 message has protections in this regard as well, as it would be an obvious way to bypass these protections (handled asynchronously after a delay, also deterministically sorted).
However, currently, if we receive a GETDATA for a transaction that we have not yet announced to the requester, we will still respond to it if it was announced to *some* other peer already (because it needs to be in `mapRelay`, which only happens on the first announcement). This is a slight privacy leak.
Thankfully, this seems easy to solve: `setInventontoryTxToSend` keeps track of the txids we have yet to announce to a peer - which almost(*) exactly corresponds to the transactions we know of that we haven't revealed to that peer. By checking whether a txid is in that set before responding to a GETDATA, we can filter these out.
(*) Locally resubmitted or rebroadcasted transactions may end up in setInventoryTxToSend while the peer already knows we have them, which could result in us incorrectly claiming we don't have such transactions if coincidentally requested right after we schedule reannouncing them, but before they're actually INVed. This is made even harder by the fact that filterInventoryKnown will generally keep known reannouncements out of setInventoryTxToSend unless it overflows (which needs 50000 INVs in either direction before it happens).
The condition for responding now becomes:
```
(not in setInventoryTxToSend) AND
(
(in relay map) OR
(
(in mempool) AND
(old enough that it could have expired from relay map) AND
(older than our last getmempool response)
)
)
```
ACKs for top commit:
naumenkogs:
utACK 2896c41
ajtowns:
ACK 2896c412fadbc03916a33028f4f50fd87ac48edb
amitiuttarwar:
code review ACK 2896c412fa
jonatack:
ACK 2896c412fadbc03916 per `git diff 2b3f101 2896c41` only change since previous review is moving the recency check up to be verified first in `FindTxForGetData`, as it was originally in 353a391 (good catch), before looking up the transaction in the relay pool.
jnewbery:
code review ACK 2896c412fadbc03916a33028f4f50fd87ac48edb
Tree-SHA512: e7d5bc006e626f60a2c108a9334f3bbb67205ace04a7450a1e4d4db1d85922a7589e0524500b7b4953762cf70554c4a08eec62c7b38b486cbca3d86321600868
|
|
|
|
|
|
|
|
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
|
|
|
|
|
|
|
|
Can be reviewed with the git diff flags
-U0 --ignore-all-space --word-diff-regex=.
|
|
|
|
consensus/validation.h, primitives/block.h and util/translation.h
cd34038cbda4864e4770734c44b18d3e01aa2a28 Switch from Optional<T> to std::optional<T> (C++17). Run clang-format. (practicalswift)
fb559c1170773360afb9d05daaccd57d18636ee9 tests: Fill fuzzing coverage gaps for functions in util/translation.h (practicalswift)
b74f3d6c452d9ad7013c70a91216220917978f66 tests: Fill fuzzing coverage gaps for functions in consensus/validation.h (practicalswift)
c0bbf8193d92ba85d62092c4fd886ff4461f65bf tests: Fill fuzzing coverage gaps for functions in primitives/block.h (practicalswift)
Pull request description:
* Fill fuzzing coverage gaps for functions in `consensus/validation.h`
* Fill fuzzing coverage gaps for functions in `primitives/block.h`
* Fill fuzzing coverage gaps for functions in `util/translation.h`
* Switch from `Optional<T>` to `std::optional<T>` (C++17). Run `clang-format`.
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 :)
Top commit has no ACKs.
Tree-SHA512: d6aa4634c3953ade173589a8239bd230eb317ef897835a8557acb73df01b25e5e17bf46f837838e59ec04c1f3d3b7d1309ba68c8a264d17b938215512c9e6085
|
|
0000ea32656833efa3d2ffd9bab66c88c83334f0 test: Add test for GetRandMillis and GetRandMicros (MarcoFalke)
fa0e5b89cf742df56c6c8f49fe9b3c54d2970a66 Add templated GetRandomDuration<> (MarcoFalke)
Pull request description:
A naive implementation of this template is dangerous, because the call site might accidentally omit the template parameter:
```cpp
template <typename D>
D GetRandDur(const D& duration_max)
{
return D{GetRand(duration_max.count())};
}
BOOST_AUTO_TEST_CASE(util_time_GetRandTime)
{
std::chrono::seconds rand_hour = GetRandDur(std::chrono::hours{1});
// Want seconds to be in range [0..1hour), but always get zero :((((
BOOST_CHECK_EQUAL(rand_hour.count(), 0);
}
```
Luckily `std::common_type` is already specialised in the standard lib for `std::chrono::duration` (https://en.cppreference.com/w/cpp/chrono/duration/common_type). And its effect seem to be that the call site must always specify the template argument explicitly.
So instead of implementing the function for each duration type by hand, replace it with a templated version that is safe to use.
ACKs for top commit:
laanwj:
Code review ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0
promag:
Code review ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0.
jonatack:
ACK 0000ea3 thanks for the improved documentation. Code review, built, ran `src/test/test_bitcoin -t random_tests -l test_suite` for the new unit tests, `git diff fa05a4c 0000ea3` since previous review:
hebasto:
ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0 with non-blocking [nit](https://github.com/bitcoin/bitcoin/pull/18781#discussion_r424924671).
Tree-SHA512: e89d46e31452be6ea14269ecbbb2cdd9ae83b4412cd14dff7d1084283092722a2f847cb501e8054394e4a3eff852f9c87f6d694fd008b3f7e8458cb5a3068af7
|
|
050e2ee6f28e7b31c167013be7313726e34084e9 test: Remove const to work around compiler error on xenial (Wladimir J. van der Laan)
Pull request description:
Fix the following error in travis:
test/validationinterface_tests.cpp:26:36: error: default initialization of an object of const type 'const BlockValidationState' without a user-provided default constructor
const BlockValidationState state_dummy;
ACKs for top commit:
MarcoFalke:
Tested ACK 050e2ee6f28e7b31c167013be7313726e34084e9 on xenial with clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
fanquake:
ACK 050e2ee6f28e7b31c167013be7313726e34084e9 - I see why we didn't hit this on master. We are installing the `clang-8` packages for the tsan job. However on the 0.20 branch we are still just installing `clang`, which is 3.8.
Tree-SHA512: 8a1d57289dbe9895ab79f81ca87b4fd723426b8d72f3a34bec9553226fba69f6dc19551c1f1d52db6c4b2652164a02ddc60f3187c3e2ad7bcacb0aaca7fa690a
|
|
|
|
|
|
|
|
|
|
746736639e6d05acdb85c866d4c605c947d4c500 [net processing] Only send a getheaders for one block in an INV (John Newbery)
Pull request description:
Headers-first is the primary method of announcement on the network. If a node fell back sending blocks by inv, it's probably for a re-org. The final block hash provided should be the highest, so send a getheaders and then fetch the blocks we need to catch up.
Sending many GETHEADERS messages to the peer would cause them to send a large number of potentially large HEADERS messages with redundant data, which is a waste of bandwidth.
ACKs for top commit:
sipa:
utACK 746736639e6d05acdb85c866d4c605c947d4c500
mzumsande:
utACK 746736639e6d05acdb85c866d4c605c947d4c500 as per ajtowns' reasoning.
naumenkogs:
utACK 7467366
ajtowns:
ACK 746736639e6d05acdb85c866d4c605c947d4c500
jonatack:
ACK 746736639e6d05acdb85c866d4c605c947d4c500
Tree-SHA512: 59e243b80d3f0873709dfacb2e4ffba34689aad7de31ec7f69a64e0e3a0756235a0150e4082ff5de823949ba4411ee1aed2344b4749b62e0eb1ea906e41f5ea9
|
|
assignment of T{}
fa1f840596554ed264d11ee3b3643bf99eb57eb5 rpcwallet: Replace pwallet-> with wallet. (MarcoFalke)
fa182a8794cbf9be1aa91d1d75e65bc7800156bc rpcwallet: Replace boost::optional<T>::emplace with simple assignment of T{} (MarcoFalke)
Pull request description:
Closes #18943
ACKs for top commit:
laanwj:
ACK fa1f840596554ed264d11ee3b3643bf99eb57eb5
ryanofsky:
Code review ACK fa1f840596554ed264d11ee3b3643bf99eb57eb5 and thanks for using a standalone commit for the fix
promag:
Code review ACK fa1f840596554ed264d11ee3b3643bf99eb57eb5.
hebasto:
ACK fa1f840596554ed264d11ee3b3643bf99eb57eb5, tested on Linux Mint 19.3.
Tree-SHA512: 0838485d1f93f737ce5bf12740669dcafeebb78dbc3fa15dbcc511edce64bf024f60f0497a04149a1e799d893d57b0c9ffe442020c1b9cfc3c69db731f50e712
|
|
Fix the following error in travis:
test/validationinterface_tests.cpp:26:36: error: default initialization of an object of const type 'const BlockValidationState' without a user-provided default constructor
const BlockValidationState state_dummy;
|
|
7777f2a4bb1f9d843bc50a4e35085cfbb2808780 miner: Avoid stack-use-after-return in validationinterface (MarcoFalke)
fa5ceb25fce2200edf6b8ebfa6d4f01ed6774b95 test: Remove UninterruptibleSleep from test and replace it by SyncWithValidationInterfaceQueue (MarcoFalke)
fa770ce7fe67685c43780e219d8232efbee0bb8e validationinterface: Rework documentation, Rename pwalletIn to callbacks (MarcoFalke)
fab6d060ce5f580db538070beec1c5518c8c777c test: Add unregister_validation_interface_race test (MarcoFalke)
Pull request description:
When a validationinterface has itself unregistered in one thread, but is about to get executed in another thread [1], there is a race:
* The validationinterface destructing itself
* The validationinterface getting dereferenced for execution
[1] https://github.com/bitcoin/bitcoin/blob/64139803f1225dab26197a20314109d37fa87d5f/src/validationinterface.cpp#L82-L83
This happens in the miner. More generally it happens everywhere where at least one thread is generating notifications and another one is unregistering a validationinterface.
This issue has been fixed in commit ab31b9d6fe7b39713682e3f52d11238dbe042c16, but the fix has not been applied to the miner.
Example where this happened in practice: https://travis-ci.org/github/bitcoin/bitcoin/jobs/675322230#L4414
ACKs for top commit:
promag:
Code review ACK 7777f2a4bb1f9d843bc50a4e35085cfbb2808780.
laanwj:
Code review ACK 7777f2a4bb1f9d843bc50a4e35085cfbb2808780
Tree-SHA512: 8087119243c71ba18a823a63515f3730d127162625d8729024278b447af29e2ff206f4840ee3d90bf84f93a2c5ab73b76c7e7044c83aa93b5b51047a166ec3d3
|
|
This is achieved by switching to a shared_ptr.
Also, switch the validationinterfaces in the tests to use shared_ptrs
for the same reason.
|
|
SyncWithValidationInterfaceQueue
For the purpose of this test the two have the same outcome, but this one
is shorter and avoids a sleep for 0.1 seconds.
|
|
|
|
This commit is (intentionally) adding a broken test. The test is broken
because it registering a subscriber object that can go out of scope
while events are still being sent.
To run the broken test and reproduce the bug:
- Remove comment /** and */
- ./configure --with-sanitizers=address
- export ASAN_OPTIONS=detect_leaks=0
- make
- while ./src/test/test_bitcoin -t validationinterface_tests/unregister_validation_interface_race --catch_system_errors=no ; do true; done
|
|
78be8d97d3d750fcfbbe509a72639b7b30b7bd18 util: Drop OpOriginal() and OpTranslated() (Hennadii Stepanov)
da16f95c3fecf4ee1e9a1dc4333b0b92cd981afd gui: Do not translate InitWarning messages in debug.log (Hennadii Stepanov)
4c9b9a4882e68835f16a52f1f0657efe47e589b5 util: Enhance Join() (Hennadii Stepanov)
fe05dd0611cfc2929a7fdec04ca5948bccf0233b util: Enhance bilingual_str (Hennadii Stepanov)
Pull request description:
This PR forces the `bitcoin-qt` to write `InitWarning()` messages to the `debug.log` file in untranslated form, i.e., in English.
On master (376294cde6b1588cb17055d8fde567eaf5848c3c):
```
$ ./src/qt/bitcoin-qt -lang=nl -debug=vladidation -printtoconsole | grep 'vladi'
Warning: Niet-ondersteunde logcategorie -debug=vladidation.
2020-05-09T12:39:59Z Warning: Niet-ondersteunde logcategorie -debug=vladidation.
2020-05-09T12:40:02Z Command-line arg: debug="vladidation"
```
With this PR:
```
$ ./src/qt/bitcoin-qt -lang=nl -debug=vladidation -printtoconsole | grep 'vladi'
Warning: Unsupported logging category -debug=vladidation.
2020-05-09T12:42:04Z Warning: Unsupported logging category -debug=vladidation.
2020-05-09T12:42:35Z Command-line arg: debug="vladidation"
```
Related to #16218.
ACKs for top commit:
laanwj:
ACK 78be8d97d3d750fcfbbe509a72639b7b30b7bd18
jonasschnelli:
utACK 78be8d97d3d750fcfbbe509a72639b7b30b7bd18
MarcoFalke:
ACK 78be8d97d3d750fcfbbe509a72639b7b30b7bd18 📢
Tree-SHA512: 48e9ecd23c4dd8ec262e3eb94f8e30944bcc9c6c163245fb837b2e0c484d4d0b4f47f7abc638c14edc27d635d340ba3ee4ba4506b062399e9cf59a1564c98755
|
|
c269e618cf53634d3cf270273ab1b0354dc3c119 Drop unused GIT_COMMIT_DATE macro (Hennadii Stepanov)
8f9f4ba5e2b738fb63c2d6cdcfdce5c169d4e88c refactor: Remove duplicated code (Hennadii Stepanov)
35f1189ea7365c1fdaf7dd9ac5e90fc8af69eae7 build: Rename BUILD_* macros and the code self-descriptive (Hennadii Stepanov)
dc1fba9389de130809834b7832d780ecf4161496 scripted-diff: Rename share/genbuild.sh macros to more meaningful ones (Hennadii Stepanov)
1e06bb68bed84e525c55022f789416ffd4793e8d Drop unused CLIENT_VERSION_SUFFIX macro (Hennadii Stepanov)
Pull request description:
This PR:
- removes unused macros and duplicated code
- renames macros in a way, that makes the code self-descriptive.
ACKs for top commit:
dongcarl:
Yup! ACK c269e618cf53634d3cf270273ab1b0354dc3c119
Tree-SHA512: c469f6269b578ccfae33d960e317eca8efaf27d49638f4c3830948c11b12ef728494d7e18c31e4a410945b7d83af5b246c7b83661b4eca17cf41ee4c4583649b
|
|
df6bde031b24112abf3a94337a2c096698acde6e test: remove glibc fdelt sanity check (fanquake)
8bf1540cc235fb8fb5330a7ae8ab638247ceb177 build: remove fdelt_chk backwards compatibility code (fanquake)
Pull request description:
https://github.com/bitcoin/bitcoin/commit/ae30d40e50e9d63d875d29d54d22147b09fc420c
The return type of [`fdelt_chk`](https://sourceware.org/git/?p=glibc.git;a=blob;f=debug/fdelt_chk.c;h=f62ce7349707cb68f55831c1c591fd7387a90258;hb=HEAD) changed from `unsigned long int` to `long int` in glibc 2.16. See [this commit](https://sourceware.org/git/?p=glibc.git;a=commit;h=ceb9e56b3d1f8c1922e0526c2e841373843460e2). Now that we require [glibc >=2.17](https://github.com/bitcoin/bitcoin/pull/17538) we can remove our back-compat code.
https://github.com/bitcoin/bitcoin/commit/ab7bce584ae02bf25e2e91aa54f9b0249427127d
While looking at the above changes, I noticed that our glibc fdelt sanity check doesn't seem to be checking anything. `fdelt_warn()` also isn't something we'd want to actually "trigger" at runtime, as doing so would cause `bitcoind` to abort.
The comments:
> // trigger: Call FD_SET to trigger __fdelt_chk. FORTIFY_SOURCE must be defined
> // as >0 and optimizations must be set to at least -O2.
suggest calling FD_SET to check the invocation of `fdelt_chk` (this is [aliased with fdelt_warn in glibc](https://sourceware.org/git/?p=glibc.git;a=blob;f=debug/fdelt_chk.c;h=f62ce7349707cb68f55831c1c591fd7387a90258;hb=HEAD)). However just calling `FD_SET()` will not necessarily cause the compiler to insert a call to `fd_warn()`.
Whether or not GCC (recent Clang should work, but may use different heuristics) inserts a call to `fdelt_warn()` depends on if the compiler can determine if the value passed in is a compile time constant (using [`__builtin_constant_p`](https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html)) and whether the value is < 0 or >= `FD_SETSIZE`. The glibc implementation is [here](https://sourceware.org/git/?p=glibc.git;a=blob;f=misc/bits/select2.h;h=7e17430ed94dd1679af10afa3d74795f9c97c0e8;hb=HEAD). This means our check should never cause a call to be inserted.
Compiling master without `--glibc-back-compat` (if you do pass `--glibc-back-compat` the outcome is still the same; however the abort will only happen with >=`FD_SETSIZE` as that is what our [fdelt_warn()](https://github.com/bitcoin/bitcoin/blob/master/src/compat/glibc_compat.cpp#L24) checks for), there are no calls to `fdelt_warn()` inserted by the compiler:
```bash
objdump -dC bitcoind | grep sanity_fdelt
...
0000000000399d20 <sanity_test_fdelt()>:
399d20: 48 81 ec 98 00 00 00 sub $0x98,%rsp
399d27: b9 10 00 00 00 mov $0x10,%ecx
399d2c: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax
399d33: 00 00
399d35: 48 89 84 24 88 00 00 mov %rax,0x88(%rsp)
399d3c: 00
399d3d: 31 c0 xor %eax,%eax
399d3f: 48 89 e7 mov %rsp,%rdi
399d42: fc cld
399d43: f3 48 ab rep stos %rax,%es:(%rdi)
399d46: 48 8b 84 24 88 00 00 mov 0x88(%rsp),%rax
399d4d: 00
399d4e: 64 48 33 04 25 28 00 xor %fs:0x28,%rax
399d55: 00 00
399d57: 75 0d jne 399d66 <sanity_test_fdelt()+0x46>
399d59: b8 01 00 00 00 mov $0x1,%eax
399d5e: 48 81 c4 98 00 00 00 add $0x98,%rsp
399d65: c3 retq
399d66: e8 85 df c8 ff callq 27cf0 <__stack_chk_fail@plt>
399d6b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
```
If you modify the sanity test to pass `-1` or `FD_SETSIZE` to `FD_SET`, you'll see calls to `fdelt_warn` inserted, and the runtime behaviour is an abort as expected.
```diff
diff --git a/src/compat/glibc_sanity_fdelt.cpp b/src/compat/glibc_sanity_fdelt.cpp
index 87140d0c7..16974bfa0 100644
--- a/src/compat/glibc_sanity_fdelt.cpp
+++ b/src/compat/glibc_sanity_fdelt.cpp
@@ -20,7 +20,7 @@ bool sanity_test_fdelt()
{
fd_set fds;
FD_ZERO(&fds);
- FD_SET(0, &fds);
+ FD_SET(FD_SETSIZE, &fds);
return FD_ISSET(0, &fds);
}
#endif
```
```bash
0000000000399d20 <sanity_test_fdelt()>:
399d20: 48 81 ec 98 00 00 00 sub $0x98,%rsp
399d27: b9 10 00 00 00 mov $0x10,%ecx
399d2c: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax
399d33: 00 00
399d35: 48 89 84 24 88 00 00 mov %rax,0x88(%rsp)
399d3c: 00
399d3d: 31 c0 xor %eax,%eax
399d3f: 48 89 e7 mov %rsp,%rdi
399d42: fc cld
399d43: f3 48 ab rep stos %rax,%es:(%rdi)
399d46: 48 c7 c7 ff ff ff ff mov $0xffffffffffffffff,%rdi
399d4d: e8 3e ff ff ff callq 399c90 <__fdelt_warn>
399d52: 0f b6 04 24 movzbl (%rsp),%eax
399d56: 83 e0 01 and $0x1,%eax
399d59: 48 8b 94 24 88 00 00 mov 0x88(%rsp),%rdx
399d60: 00
399d61: 64 48 33 14 25 28 00 xor %fs:0x28,%rdx
399d68: 00 00
399d6a: 75 08 jne 399d74 <sanity_test_fdelt()+0x54>
399d6c: 48 81 c4 98 00 00 00 add $0x98,%rsp
399d73: c3 retq
399d74: e8 77 df c8 ff callq 27cf0 <__stack_chk_fail@plt>
399d79: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
```
```bash
src/bitcoind
*** buffer overflow detected ***: src/bitcoind terminated
Aborted
```
I think the test should should be removed and replaced (if possible) with additional checks in security-check.py. I was thinking about adding a version of [this script](https://github.com/fanquake/core-review/blob/master/fortify.py) as part of the output, but that needs more thought. I'll address this in a follow up.
ACKs for top commit:
laanwj:
ACK df6bde031b24112abf3a94337a2c096698acde6e
Tree-SHA512: d8b3af4f4eb2d6c767ca6e72ece51d0ab9042e1bbdfcbbdb7ad713414df21489ba3217662b531b8bfdac0265d2ce5431abfae6e861b6187d182ff26c6e59b32d
|
|
9f59dde9740d065118bdddde75ef9f4e4603a7b1 rpc: Relock wallet only if most recent callback (João Barbosa)
a2e6db5c4f1bb52a8814102b628e51652493d06a rpc: Add mutex to guard deadlineTimers (João Barbosa)
Pull request description:
This PR fixes an early relocking race condition from #18811 where old relock callback runs after new wallet unlock code and nRelockTime update but before rpcRunLater call, causing early relock and incorrect nRelockTime time
Issue introduced in #18487.
Fixes #18811.
ACKs for top commit:
MarcoFalke:
ACK 9f59dde9740d065118bdddde75ef9f4e4603a7b1
ryanofsky:
Code review ACK 9f59dde9740d065118bdddde75ef9f4e4603a7b1. No changes since last review except squashing commits.
jonatack:
ACK 9f59dde9740d065118bdddde75ef
Tree-SHA512: 2f7fc03e5ab6037337f2d82dfad432495cc337c77d07c968ee2355105db6292f24543c03456f5402e0e759577a4327758f9372f7ea29de6d56dc3695fda9b379
|
|
a8b5f1b133d4f23975a3fbfb7a415b17261466ee gui: Fix manual coin control with multiple wallets loaded (João Barbosa)
Pull request description:
This PR ensures each loaded wallet has a dedicated coin control in the send view which is manipulated by the coin control dialog.
This is an alternative to #17457. Two main differences are:
- scope reduced - no unnecessary changes unrelated to the fix;
- approach taken - coin control instance now belongs to the send view.
All problems raised in #17457 reviews no longer apply due to the approach taken - https://github.com/bitcoin/bitcoin/pull/17457#pullrequestreview-319297589 and https://github.com/bitcoin/bitcoin/pull/17457#issuecomment-555920829)
No change in behavior if only one wallet is loaded.
Closes #15725.
ACKs for top commit:
jonasschnelli:
utACK a8b5f1b133d4f23975a3fbfb7a415b17261466ee
ryanofsky:
Code review ACK a8b5f1b133d4f23975a3fbfb7a415b17261466ee. Code changes are very straightforward, just replacing global CCoinControl object with SendCoinsDialog member. Not sure if this means coin control settings are reset between payments. It would be good to note in the PR description or release notes if single wallet behavior is affected
hebasto:
ACK a8b5f1b133d4f23975a3fbfb7a415b17261466ee
Sjors:
tACK a8b5f1b133d4f23975a3fbfb7a415b17261466ee
Tree-SHA512: 3ad9c51bab6f28ec0e90efbd6f43fa510c81dafb2eff0b8c3724efcee3e030054a10be013e27cefe35763374c5f6d7af8c02658736964f733d7e38b646b5df65
|
|
e8123eae40eb264bbb71007d0eb074901f0e2fe5 gui: Fix itemWalletAddress leak when not tree mode (João Barbosa)
Pull request description:
Taken from #17457, the first commit is a similar to 88a94f7bb8ba2b0257315d70717f9af928ca6561 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked.
ACKs for top commit:
jonasschnelli:
utACK e8123eae40eb264bbb71007d0eb074901f0e2fe5
hebasto:
ACK e8123eae40eb264bbb71007d0eb074901f0e2fe5, tested on Linux Mint 19.3.
Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e
|
|
839add193b13c17a40f42ff69d973caeb800d3f2 build: Enable -Wsuggest-override (Hennadii Stepanov)
de5e91c3034f320f84ee0308a3c31659635d136a refactor: Add BerkeleyDatabaseVersion() function (Hennadii Stepanov)
Pull request description:
From GCC [docs](https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/Warning-Options.html):
> `-Wsuggest-override`
> Warn about overriding virtual functions that are not marked with the override keyword.
~This PR is based on #16722 (the first commit).~ See: https://github.com/bitcoin/bitcoin/pull/16722#issuecomment-584111086
ACKs for top commit:
fanquake:
ACK 839add193b13c17a40f42ff69d973caeb800d3f2
vasild:
ACK 839add193
practicalswift:
ACK 839add193b13c17a40f42ff69d973caeb800d3f2 assuming Travis is happy: patch looks correct
Tree-SHA512: 1e8cc085da30d41536deff9b181962c1882314ab252c2ad958294087ae1e5a0dfa4886bdbe36f21cf6ae71df776a8420f349f007d4b5b49fd79ba98ce308965a
|
|
|
|
Headers-first is the primary method of announcement on the network. If a
node fell back sending blocks by inv, it's probably for a re-org. The
final block hash provided should be the highest, so send a getheaders
and then fetch the blocks we need to catch up.
|
|
|
|
|
|
|
|
e3047edfb63c3d098cb56ba9f9a1e7e0a795d552 test: use p2p constants in denial of service tests (fanquake)
25d8264c95eaf98a66df32addb0bf32d795a35bd p2p: add MAX_FEELER_CONNECTIONS constant (tryphe)
Pull request description:
Extracted from #16003.
ACKs for top commit:
naumenkogs:
utACK e3047ed
Tree-SHA512: 14fc15292be4db2e825a0331dd189a48713464f622a91c589122c1a7135bcfd37a61e64af1e76d32880ded09c24efd54d3c823467d6c35367a380e0be33bd35f
|
|
23083856a551ca13e8b142791c296ecb25cc4e7f [test] Add test for cfcheckpt (Jim Posen)
f9e00bb25ac4039056808affeb5ffa86a2c317fe [net processing] Message handling for getcfcheckpt. (Jim Posen)
9ccaaba11e94571fe984857494042ac292c17156 [init] Add -peerblockfilters option (Jim Posen)
Pull request description:
Serve cfcheckpt messages if basic block filter index is enabled and `-peercfilters` is set.
`NODE_COMPACT_FILTERS` is not signaled to peers, but functionality can be used for testing and serving pre-configured clients.
ACKs for top commit:
jonatack:
Code review re-ACK 23083856a551ca13e8b142791c296ecb25cc4e7f the only change since my review @ 967e2b1 is an update required for #16224 that was merged yesterday.
fjahr:
re-ACK 23083856a551ca13e8b142791c296ecb25cc4e7f
jkczyz:
re-ACK 23083856a551ca13e8b142791c296ecb25cc4e7f
ariard:
re-Code Review ACK 2308385
clarkmoody:
Tested ACK 23083856a
MarcoFalke:
re-ACK 23083856a5 🌳
theStack:
ACK https://github.com/bitcoin/bitcoin/commit/23083856a551ca13e8b142791c296ecb25cc4e7f
Tree-SHA512: 8c751bbd7d1c31a413096462ae025c3d2f3163c7016cbec472a5f5ec267f8dd19a2dfc4d749876d7409c1db546e6fdd16461c6863effcfa0d3e993edcfa92a08
|
|
|
