| Age | Commit message (Collapse) | Author |
|---|
|
Can be reviewed with the git diff options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --ignore-all-space
|
|
fa9ee52556f493e4a896e2570ca1a3102d777d9a doc: Add doxygen comment to IsRBFOptIn (MarcoFalke)
faef4fc9b4990e563022b6ab595cb02c4060c216 Remove mempool global from interfaces (MarcoFalke)
fa831684e54783f6b40533ca218eb7636bdae667 refactor: Add IsRBFOptInEmptyMempool (MarcoFalke)
Pull request description:
The chain interface has an `m_node` member, which has a pointer to the mempool global. Use the pointer instead of the global to prepare the removal of the mempool global. See #19556
ACKs for top commit:
jnewbery:
utACK fa9ee52556
darosior:
ACK fa9ee52
hebasto:
re-ACK fa9ee52556f493e4a896e2570ca1a3102d777d9a, since my [previous](https://github.com/bitcoin/bitcoin/pull/19848#pullrequestreview-482403942) review:
Tree-SHA512: 11b4c1446f0860a743fdaa67f95c52bf0262d0a4f888be0eaf07ee497448965d32be414111bf016bd568f2989cde923430e3a3889e224057b73c499f06de7199
|
|
networks
86d4cf42d97abf4c436d1eabf29e2ed150f69c1e Increase the ip address relay branching factor for unreachable networks (Pieter Wuille)
Pull request description:
Onion addresses propagate very badly among the IPv4/IPv6 network, resulting
in difficulty for those to find each other.
The branching factor 1 is probably so low that propagations die out before
they reach another onion peer. Increase it to 1.5 on average.
ACKs for top commit:
practicalswift:
ACK 86d4cf42d97abf4c436d1eabf29e2ed150f69c1e -- patch looks correct
naumenkogs:
ACK 86d4cf4
jonatack:
ACK 86d4cf42d97abf4c436d1eabf29e2ed150f69c1e. Code review, built and running with some sanity check logging. `RelayAddress()` is called by `ProcessMessage() ADDR` msg handling, from within the loop while processing each new address to relay it to a limited number of other nodes. According to git blame, the line setting `nRelayNodes` hasn't been touched since 2016 in e736772c56a *Move network-msg-processing code out of main to its own file*, which moved the line but otherwise did not change it. Running a mixed clearnet/onion node with this patch and the logging below, I'm only seeing values of `fReachable 1, nRelayNodes 2`. IIUC, I need to use the settings in `init.cpp` that call `SetReachable(*, false)`. *Edit:* with `onlynet=onion` am now seeing entries of `fReachable 0` with `nRelayNodes` values of 1 and 2.
vasild:
ACK 86d4cf42d
Tree-SHA512: 22391e16d60bcfdec9a9336728da39d68a24a183b3d1b0e8fbc038d265ca6ddf71d16db018f3678745fd9f3e9281049e42197fa0a29124833c50a9170ed6f793
|
|
ac2ff4fb1e06270cf17727f90599c9f3a55ddd5a refactor: Avoid duplicate map lookup in ScriptToAsmStr (João Barbosa)
Pull request description:
Simple change that avoids a duplicate (unnecessary) `mapSigHashTypes` lookup.
ACKs for top commit:
laanwj:
re-ACK ac2ff4fb1e06270cf17727f90599c9f3a55ddd5a
Tree-SHA512: 7e7f5af51c1acd7a42af273e5ee5e2faddd250ba8b8f63ccb3172d95f153ae391b2816b79564b856571af52dc2a767b5736a5d10ffb5cd2c540cd9832bf86419
|
|
|
|
|
|
Co-authored-by: John Newbery <jonnynewbs@gmail.com>
|
|
|
|
and `-getinfo`
581b343d5bf517510ab0236583ca96628751177d Add in/out connections to cli -getinfo (Jon Atack)
d9cc13e88d096c1a171159c01cbb96444f7f8d7f UNIX_EPOCH_TIME fixup in rpc getnettotals (Jon Atack)
1ab49b81cf32b6ef9e312a0a8ac45c68a3262f0d Add in/out connections to rpc getnetworkinfo (Jon Atack)
Pull request description:
This is basic info that is present in the GUI that I've been wishing to have exposed via the RPC and CLI without needing a bash workaround or script. For human users it would also be useful to have it in `-getinfo`.
`bitcoin-cli getnetworkinfo`
```
"connections": 15,
"connections_in": 6,
"connections_out": 9,
```
`bitcoin-cli -getinfo`
```
"connections": {
"in": 6,
"out": 9,
"total": 15
},
```
Update the tests, RPC help, and release notes for the changes. Also fixup the `getnettotals` timemillis help while touching `rpc/net.cpp`.
-----
Reviewers can manually test this PR by [building from source](https://jonatack.github.io/articles/how-to-compile-bitcoin-core-and-run-the-tests), launching bitcoind, and then running `bitcoin-cli -getinfo`, `bitcoin-cli getnetworkinfo`, `bitcoin-cli help getnetworkinfo`, and `bitcoin-cli help getnettotals` (for the UNIX epoch time change).
ACKs for top commit:
eriknylund:
> tACK [581b343](https://github.com/bitcoin/bitcoin/commit/581b343d5bf517510ab0236583ca96628751177d) on master at [a0a422c](https://github.com/bitcoin/bitcoin/commit/a0a422c34cfd6514d0cc445bd784d3ee1a2d1749), ran unit & functional tests and and confirmed changes on an existing datadir ✌️
benthecarman:
tACK `581b343`
willcl-ark:
tACK for 581b343d5bf517510ab0236583ca96628751177d, this time rebased onto master at 862fde88be706adb20a211178253636442c3ae00.
shesek:
tACK `581b343`. This provides what I needed, thanks!
n-thumann:
tACK 581b343 on master at a0a422c, ran unit & functional tests and and confirmed changes on an existing datadir ✌️
Tree-SHA512: 08dd3ac8fefae401bd8253ff3ac027603c528eeccba53cedcb127771316173a7052fce44af8fa33ac98ebc4cf2a2b11cdefd949995d55e9b9a5942b876d00dc5
|
|
020f0519ec66d9626255b938e1c6c3f7f9aa4017 refactor: CTxMemPool::IsUnbroadcastTx() requires CTxMemPool::cs lock (Hennadii Stepanov)
7c4bd0387a01a0c3e2938d530dba3c882e4d8f2b refactor: CTxMemPool::GetTotalTxSize() requires CTxMemPool::cs lock (Hennadii Stepanov)
fa5fcb032b6ed04c49ee465235288b8059fa805e refactor: CTxMemPool::ClearPrioritisation() requires CTxMemPool::cs lock (Hennadii Stepanov)
7140b31b90cbd84d75eedb3e395d0d55f83b5b95 refactor: CTxMemPool::ApplyDelta() requires CTxMemPool::cs lock (Hennadii Stepanov)
66e47e5e506043fbb9b4e487b44bf992985709c9 refactor: CTxMemPool::UpdateChild() requires CTxMemPool::cs lock (Hennadii Stepanov)
939807768acd508932f2efabee660d56324a73df refactor: CTxMemPool::UpdateParent() requires CTxMemPool::cs lock (Hennadii Stepanov)
Pull request description:
This is another step to transit `CTxMemPool::cs` from `RecursiveMutex` to `Mutex`.
Split out from #19306.
Only trivial thread safety annotations and lock assertions added. No new locks. No behavior change.
Refactoring `const uint256` to `const uint256&` was [requested](https://github.com/bitcoin/bitcoin/pull/19647#discussion_r468471022) by **promag**.
Please note that now, since #19668 has been merged, it is safe to apply `AssertLockHeld()` macros as they do not swallow compile time Thread Safety Analysis warnings.
ACKs for top commit:
promag:
Core review ACK 020f0519ec66d9626255b938e1c6c3f7f9aa4017.
jnewbery:
Code review ACK 020f0519ec66d9626255b938e1c6c3f7f9aa4017
vasild:
ACK 020f0519e
Tree-SHA512: a31e389142d5a19b25fef0aaf1072a337278564528b5cc9209df88ae548a31440e1b8dd9bae0169fd7aa59ea06e22fe5e0413955386512b83ef1f3e7d941e890
|
|
|
|
f1ee37319a7a211e5fb325406d62db5b61dbd30e wallet: Reload previously loaded wallets on GUI startup (Andrew Chow)
Pull request description:
Enable the GUI to also use the load_on_startup feature. Wallets loaded in the GUI always have load_on_startup=true. When they are unloaded from the GUI, load_on_startup=false.
To facilitate this change, UpdateWalletSetting is moved into the wallet module and called from within LoadWallet, RemoveWallet, and Createwallet. This change does not actually touch the GUI code but rather the wallet functions that are shared between the GUI and RPC.
ACKs for top commit:
jonasschnelli:
Tested ACK f1ee37319a7a211e5fb325406d62db5b61dbd30e - works as expected. Wallets loaded via bitcoin-cli (in `-server` mode) or through the RPC console won't be loaded on startup but wallets loaded via the GUI menu will.
kristapsk:
ACK f1ee37319a7a211e5fb325406d62db5b61dbd30e, I have tested the code.
Tree-SHA512: f5b44aa763cf761d919015c5fbc0600b72434aa71e3b57007fd7530a29c3da1a9a0c98c4f22cb6cdffba61150a31170056a7d4737625e7b76f6958f3d584da8c
|
|
warning
7984c39be11ca04460883365e1ae2a496aaa6c0e test framework: serialize/deserialize inv type as unsigned int (Jon Atack)
407175e0c2bc797599ebd9c0a1f2ec89ad7af136 p2p: change CInv::type from int to uint32_t (Jon Atack)
Pull request description:
Fixes UBSan implicit-integer-sign-change issue per https://github.com/bitcoin/bitcoin/pull/19610#issuecomment-680686460.
Credit to Crypt-iQ for finding and reporting the issue and to vasild for the original review suggestion in https://github.com/bitcoin/bitcoin/pull/19590#pullrequestreview-455788826.
Closes #19678.
ACKs for top commit:
laanwj:
ACK 7984c39be11ca04460883365e1ae2a496aaa6c0e
MarcoFalke:
ACK 7984c39be11ca04460883365e1ae2a496aaa6c0e 🌻
vasild:
ACK 7984c39be
Tree-SHA512: 59f3a75f40ce066ca6f0bb1927197254238302b4073af1574bdbfe6ed580876437be804be4e47d51467d604f0d9e3a5875159f7f2edbb2351fdb2bb9465100b5
|
|
752e6ad5336d5af0db9fe16d24c0c6aa25b74a3f Protect localhost and block-relay-only peers from eviction (Suhas Daftuar)
Pull request description:
Onion peers are disadvantaged under our eviction criteria, so prevent eventual
eviction of them in the presence of contention for inbound slots by reserving
some slots for localhost peers (sorted by longest uptime).
Block-relay-only connections exist as a protection against eclipse attacks, by
creating a path for block propagation that may be unknown to adversaries.
Protect against inbound peer connection slot attacks from disconnecting such
peers by attempting to protect up to 8 peers that are not relaying transactions
but have provided us with blocks.
Thanks to gmaxwell for suggesting these strategies.
ACKs for top commit:
laanwj:
Code review ACK 752e6ad5336d5af0db9fe16d24c0c6aa25b74a3f
Tree-SHA512: dbf089c77c1f747aa1dbbbc2e9c2799c628028b0918d0c336d8d0e5338acedd573b530eb3b689c7f603a17221e557268a9f5c3f585f204bfb12e5d2e76de39a3
|
|
eb1c5d090f4ef844dabc1e8bf3864c1afe1c791c [doc] Follow developer notes, add comment about missing default. (Amiti Uttarwar)
d5a57cef62ee9e9d30f7e3b80e178149ceeef67c [doc] Describe connection types in more depth. (Amiti Uttarwar)
4829b6fcc6489b445f80689af6c2a1a919f176b1 [refactor] Simplify connection type logic in ThreadOpenConnections (Amiti Uttarwar)
1e563aed785565af6b7aed7f7399c52205d8f19c [refactor] Simplify check for block-relay-only connection. (Amiti Uttarwar)
da3a0be61b025224231206cb4656e420453bfdeb [test] Add explicit tests that connection types get set correctly (Amiti Uttarwar)
1d74fc7df621b31d1b8adc9d7f53e7478d6e40b5 [trivial] Small style updates (Amiti Uttarwar)
ff6b9081add3a40d53b1cc1352b57eeb46e41d45 [doc] Explain address handling logic in process messages (Amiti Uttarwar)
dff16b184b1428a068d144e5e4dde7595b4729c0 [refactor] Restructure logic to check for addr relay. (Amiti Uttarwar)
a6ab1e81f964df131cfa0e11e07bedb3283b823f [net] Remove unnecessary default args on OpenNetworkConnection (Amiti Uttarwar)
8d6ff46f55f373e344278ab3f1ac27b1dba36623 scripted-diff: Rename `OUTBOUND` ConnectionType to `OUTBOUND_FULL_RELAY` (Amiti Uttarwar)
Pull request description:
This PR addresses outstanding review comments from #19316. It further simplifies `net.cpp` complexity and adds documentation about design goals about different connection types.
ACKs for top commit:
naumenkogs:
ACK eb1c5d090f4ef844dabc1e8bf3864c1afe1c791c
laanwj:
Code review ACK eb1c5d090f4ef844dabc1e8bf3864c1afe1c791c
Tree-SHA512: 2fe14e428404c95661e5518c8c90db07ab5b9ebb1bac921b3bdf82b181f444fae379f8fc0a2b619e6b4693f01c55bd246fbd8c8eedadd96849a30de3161afee5
|
|
0bbe26a1af2aab2287b18048f80b3f70e63e0044 wallet: filter for keys only before record deser in salvage (Andrew Chow)
544e12a4e81633d222574eec253a1ff292d3c4a5 walletdb: Add KeyFilterFn to ReadKeyValue (Andrew Chow)
Pull request description:
When salvaging a wallet, the only things that matter are the private keys. It is not necessary to attempt to deserialize any other records, especially if those records are corrupted too.
This PR adds a `KeyFilterFn` function callback to `ReadKeyValue` that salvage uses to filter for only the records that it wants. Of course doing it this way also lets us do other filters in the future from other places should we so desire.
ACKs for top commit:
ryanofsky:
Code review ACK 0bbe26a1af2aab2287b18048f80b3f70e63e0044. Looks great! This should make the recovery code more robust. Normally it'd be good to have a test case for the problem this fixes, but Marco already wrote one in #19078, so I think we're covered
laanwj:
Code review ACK 0bbe26a1af2aab2287b18048f80b3f70e63e0044
Tree-SHA512: 8e3ee283a22a79273915711c4fb751f3c9b02ce94e6bf08dc468f1cfdf9fac35c693bbfd2435ce43c3a06c601b9b0a67e209621f6814bedfe3bc7a7ccc37bb01
|
|
e36f802fa4916586b53a989c0848389ad838b846 lint: add C++ code linter (fanquake)
c4be50fea31e85f0e7151ed5ecaba531f6f929db remove usage of boost::bind (fanquake)
Pull request description:
`boost::bind` usage was removed in #13743. However a new usage snuck in as
part of 2bc4c3eaf96f5f8490fc79280422916c5d14cde3 (#15225).
ACKs for top commit:
hebasto:
ACK e36f802fa4916586b53a989c0848389ad838b846
practicalswift:
ACK e36f802fa4916586b53a989c0848389ad838b846 -- patch looks correct
Tree-SHA512: 2b0387c5443c184bcbf7df4849db1ed1296ff82c7b4ff0aff18334a400e56a472a972d18234d3866531a088d7a8da64688e58dc9f15daaad4048697c759d55ce
|
|
|
|
|
|
Consolidate the logic to determine connection type into one conditional to
clarify how they are chosen.
|
|
Previously we deduced it was a block-relay-only based on presence of the
m_tx_relay structure. Now we have the ability to identify it directly via a
connection type accessor function.
|
|
|
|
|
|
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
|
|
We previously identified if we relay addresses to the connection by checking
for the existence of the m_addr_known data structure. With this commit, we
answer this question based on the connection type.
IsAddrRelayPeer() checked for the existence of the m_addr_known
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/OUTBOUND, /OUTBOUND_FULL_RELAY, /g' src/net.h
sed -i 's/ConnectionType::OUTBOUND/ConnectionType::OUTBOUND_FULL_RELAY/g' src/test/net_tests.cpp src/test/fuzz/process_message.cpp src/test/fuzz/process_messages.cpp src/net.cpp src/test/denialofservice_tests.cpp src/net.h src/test/fuzz/net.cpp
-END VERIFY SCRIPT-
|
|
Onion peers are disadvantaged under our eviction criteria, so prevent eventual
eviction of them in the presence of contention for inbound slots by reserving
some slots for localhost peers (sorted by longest uptime).
Block-relay-only connections exist as a protection against eclipse attacks, by
creating a path for block propagation that may be unknown to adversaries.
Protect against inbound peer connection slot attacks from disconnecting such
peers by attempting to protect up to 8 peers that are not relaying transactions
but appear to be full-nodes, sorted by recency of last delivered block.
Thanks to gmaxwell for suggesting these strategies.
|
|
413e0d1d31ede6a9b539d63ec814b6e8044e35e2 Avoid callback when -blocknotify is empty (João Barbosa)
Pull request description:
ACKs for top commit:
MarcoFalke:
ACK 413e0d1d31ede6a9b539d63ec814b6e8044e35e2
practicalswift:
ACK 413e0d1d31ede6a9b539d63ec814b6e8044e35e2 -- patch looks correct
laanwj:
Code review ACK 413e0d1d31ede6a9b539d63ec814b6e8044e35e2
Tree-SHA512: 915e796666b4e74dbb029ba5436e5573a4b881aad9e118f737bcff4024528b7ff3b00dd035138f63d30963cfd66195f6e53a2dbe429ee28cb6f0b9cc47218ecf
|
|
d780293e1ee0f9e66bd2d88914694c17f9aaa0ca net: improve nLastBlockTime and nLastTXTime documentation (Jon Atack)
Pull request description:
Follow-up to #19731 to help alleviate confusion around `nLastBlockTime` and `nLastTXTime`, now also provided by the JSON-RPC API as `last_block` and `last_transaction` in `getpeerinfo` output.
Thanks to John Newbery, credited in the commit, and to Dave Harding and Adam Jonas during discussions on how to best explain these in this week's Optech newsletter.
ACKs for top commit:
practicalswift:
ACK d780293e1ee0f9e66bd2d88914694c17f9aaa0ca
MarcoFalke:
ACK d780293e1ee0f9e66bd2d88914694c17f9aaa0ca
harding:
ACK d780293e1ee0f9e66bd2d88914694c17f9aaa0ca . The added documentation matches my reading of the code and answers a question I had after seeing #19731
0xB10C:
ACK d780293e1ee0f9e66bd2d88914694c17f9aaa0ca
Tree-SHA512: 72d47cf50a099913c7e4753cb80e11785b26fb66fa3a8b6c382fde4ea725116f3d215f93d32a567246d269768e66159f8dcf017a1bbc6d5f2489a35f81c316fa
|
|
fb56d37612dea6666e7da73d671311a697570dae p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing (John Newbery)
aa3621385ee66c9dde5c632c0a79fba3a6ea2d62 test: use CInv::MSG_WITNESS_TX flag in p2p_segwit (Jon Atack)
24ee4f01eadb870435712950a1364cf0def06e9f p2p: make gtxid(.hash) and fAlreadyHave localvars const (Jon Atack)
b1c855453bf2634e7fd9b53c4a76a8536fc9865d p2p: use CInv block message helpers in net_processing.cpp (Jon Atack)
acd66421671e42a58e8e067868e1ab86268e3231 [net processing] Change AlreadyHaveTx() to take a GenTxid (John Newbery)
5fdfb80b861e0de3fcf8a57163b3f52af4b2df3b [net processing] Change AlreadyHaveBlock() to take block_hash argument (John Newbery)
430e183b89d00b4148f0b77a6fcacca2cd948202 [net processing] Remove mempool argument from AlreadyHaveBlock() (John Newbery)
42ca5618cae0fd9ef97d2006b17d896bc58cc17c [net processing] Split AlreadyHave() into separate block and tx functions (John Newbery)
39f1dc944554218911b0945fff7e6d06f3dab284 p2p: remove nFetchFlags from NetMsgType TX and INV processing (Jon Atack)
471714e1f024fb3b4892a7a8b34a76b83a13fa19 p2p: add CInv block message helper methods (Jon Atack)
Pull request description:
Building on #19590 and the recent `wtxid` and `GenTxid` changes, this is a refactoring and cleanup PR to simplify and improve some of the net processing code.
Some of the diffs are best reviewed with `-w` to ignore spacing.
Co-authored by John Newbery.
ACKs for top commit:
laanwj:
Code review ACK fb56d37612dea6666e7da73d671311a697570dae
jnewbery:
utACK fb56d37612dea6666e7da73d671311a697570dae
vasild:
ACK fb56d3761
Tree-SHA512: ba39b58e6aaf850880a842fe5f6295e9f1870906ef690206acfc17140aae2ac854981e1066dbcd4238062478762fbd040ef772fdc2c50eea6869997c583e6a6d
|
|
c276df775914e4e42993c76e172ef159e3b830d4 zmq: enable tcp keepalive (mruddy)
Pull request description:
This addresses https://github.com/bitcoin/bitcoin/issues/12754.
These changes enable node operators to address the silent dropping (by network middle boxes) of long-lived low-activity ZMQ TCP connections via further operating system level TCP keepalive configuration. For example, ZMQ sockets that publish block hashes can be affected in this way due to the length of time it sometimes takes between finding blocks (e.g.- sometimes more than an hour).
Prior to this patch, operating system level TCP keepalive configurations would not take effect since the SO_KEEPALIVE option was not enabled on the underlying socket.
There are additional ZMQ socket options related to TCP keepalive that can be set. However, I decided not to implement those options in this changeset because doing so would require adding additional bitcoin node configuration options, and would not yield a better outcome. I preferred a small, easily reviewable patch that doesn't add a bunch of new config options, with the tradeoff that the fine tuning would have to be done via well-documented operating system specific configurations.
I tested this patch by running a node with:
`./src/qt/bitcoin-qt -regtest -txindex -datadir=/tmp/node -zmqpubhashblock=tcp://127.0.0.1:28332 &`
and connecting to it with:
`python3 ./contrib/zmq/zmq_sub.py`
Without these changes, `ss -panto | grep 28332 | grep ESTAB | grep bitcoin` will report no keepalive timer information. With these changes, the output from the prior command will show keepalive timer information consistent with the configuration at the time of connection establishment, e.g.-: `timer:(keepalive,119min,0)`.
I also tested with a non-TCP transport and did not witness any adverse effects:
`./src/qt/bitcoin-qt -regtest -txindex -datadir=/tmp/node -zmqpubhashblock=ipc:///tmp/bitcoin.block &`
ACKs for top commit:
adamjonas:
Just to summarize for those looking to review - as of c276df775914e4e42993c76e172ef159e3b830d4 there are 3 tACKs (n-thumann, Haaroon, and dlogemann), 1 "looks good to me" (laanwj) with no NACKs or any show-stopping concerns raised.
jonasschnelli:
utACK c276df775914e4e42993c76e172ef159e3b830d4
Tree-SHA512: b884c2c9814e97e666546a7188c48f9de9541499a11a934bd48dd16169a900c900fa519feb3b1cb7e9915fc7539aac2829c7806b5937b4e1409b4805f3ef6cd1
|
|
Enable the GUI to also use the load_on_startup feature.
Wallets loaded in the GUI always have load_on_startup=true.
When they are unloaded from the GUI, load_on_startup=false.
To facilitate this change, UpdateWalletSetting is moved into the wallet
module and called from within LoadWallet, RemoveWallet, and
Createwallet. This change does not actually touch the GUI code but
rather the wallet functions that are shared between the GUI and RPC.
|
|
Co-authored-by: John Newbery <john@johnnewbery.com>
|
|
No change in behavior, the lock is already held at call sites.
|
|
No change in behavior, the lock is already held at call sites.
|
|
No change in behavior, the lock is already held at call sites.
Also `const uint256` refactored to `const uint256&`.
|
|
No change in behavior, the lock is already held at call sites.
Also `const uint256` refactored to `const uint256&`.
|
|
No change in behavior, the lock is already held at call sites.
|
|
No change in behavior, the lock is already held at call sites.
|
|
ea74e10acf17903e44c85e3678853414653dd4e1 doc: Add best practice for annotating/asserting locks (Hennadii Stepanov)
2ee7743fe723227f2ea1b031eddb14fc6863f4c8 sync.h: Make runtime lock checks require compile-time lock checks (Anthony Towns)
23d71d171e6e22ba5e4a909d597a54595b2a2c1f Do not hide compile-time thread safety warnings (Hennadii Stepanov)
3ddc150857178bfb1c854c05bf9b526777876f56 Add missed thread safety annotations (Hennadii Stepanov)
af9ea55a72c94678b343f5dd98dc78f3a3ac58cb Use LockAssertion utility class instead of AssertLockHeld() (Hennadii Stepanov)
Pull request description:
On the way of transit from `RecursiveMutex` to `Mutex` (see #19303) it is crucial to have run-time `AssertLockHeld()` assertion that does _not_ hide compile-time Clang Thread Safety Analysis warnings.
On master (65e4ecabd5b4252154640c7bac38c92a3f3a7018) using `AssertLockHeld()` could hide Clang Thread Safety Analysis warnings, e.g., with the following patch applied:
```diff
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -607,7 +607,7 @@ public:
void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs);
- void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
+ void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs);
```
Clang compiles the code without any thread safety warnings.
See "Add missed thread safety annotations" commit for the actual thread safety warnings that are fixed in this PR.
ACKs for top commit:
MarcoFalke:
ACK ea74e10acf 🎙
jnewbery:
ACK ea74e10acf17903e44c85e3678853414653dd4e1
ajtowns:
ACK ea74e10acf17903e44c85e3678853414653dd4e1
Tree-SHA512: 8cba996e526751a1cb0e613c0cc1b10f027a3e9945fbfb4bd30f6355fd36b9f9c2e1e95ed3183fc254b42df7c30223278e18e5bdb5e1ef85db7fef067595d447
|
|
3340dbadd38f5624642cf0e14dddbe6f83a3863b Remove -zapwallettxes (Andrew Chow)
Pull request description:
It's not clear what use there is to keeping `-zapwallettxes` given that it's intended usage has been superseded by `abandontransaction`. So this removes it outright.
Alternative to #19700
ACKs for top commit:
meshcollider:
utACK 3340dbadd38f5624642cf0e14dddbe6f83a3863b
fanquake:
ACK 3340dbadd38f5624642cf0e14dddbe6f83a3863b - remaining manpage references will get cleaned up pre-release.
Tree-SHA512: 3e58e1ef6f4f94894d012b93e88baba3fb9c2ad75b8349403f9ce95b80b50b0b4f443cb623cf76c355930db109f491b3442be3aa02972e841450ce52cf545fc8
|
|
-zapwallettxes is made a hidden option to inform users that it is
removed and they should be using abandontransaction to do the stuck
transaction thing.
|
|
(mining,zmq,rpcdump)
fa3d9ce3254882c545d700990fe8e9a678f31eed rpc: Assert that RPCArg names are equal to CRPCCommand ones (rpcdump) (MarcoFalke)
fa32c1d5ec25bc53bf989a8ae68e688593d2859d rpc: Assert that RPCArg names are equal to CRPCCommand ones (zmq) (MarcoFalke)
faaa46dc204d6d714f71dbc6f0bf02215dba0f0f rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining) (MarcoFalke)
fa93bc14c7411a108dd024d391344fabf0f76369 rpc: Remove unused return type from appendCommand (MarcoFalke)
Pull request description:
This is split out from #18531 to just touch the RPC methods in misc. Description from the main pr:
### Motivation
RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.
### Changes
The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
fjahr:
tested ACK fa3d9ce3254882c545d700990fe8e9a678f31eed
promag:
Code review ACK fa3d9ce3254882c545d700990fe8e9a678f31eed.
Tree-SHA512: 068ade4b55cc195868d53b7f9a27151d45b440857bb069e261a49d102a49a38fdba5d68868516a1d66a54a73ba34681362f934ded7349e894042bde873b75719
|
|
boost::bind usage was removed in #13743. However a new usage snuck in as
part of 2bc4c3eaf96f5f8490fc79280422916c5d14cde3 (#15225).
|
|
manually selected coins
6d1f51343cf11b07cd401fbd0c5bc3603e185a0e [rpc] fundrawtransaction, walletcreatefundedpsbt lock manually selected coins (Sjors Provoost)
Pull request description:
When using `fundrawtransaction` and `walletcreatefundedpsbt` with `lockUnspents`, it would only lock automatically selected coins, not manually selected coins. That doesn't make much sense to me if the goal is to prevent accidentally double-spending yourself before you broadcast a transaction.
Note that when creating a transaction, manually selected coins are automatic "unlocked" (or more accurately: the lock is ignored). Earlier versions of this PR introduced an error when a locked coin is manually selected, but this idea was abandoned after some discussion. An application that uses this RPC should either rely on automatic coin selection (with `lockUnspents`) or handle lock concurrency itself with manual coin selection. In particular it needs to make sure to avoid/pause calls with automatic coin selection between calling `lockunspent` and the subsequent spending RPC.
See #7518 for historical background.
ACKs for top commit:
meshcollider:
Code review ACK 6d1f51343cf11b07cd401fbd0c5bc3603e185a0e
fjahr:
Code review ACK 6d1f51343cf11b07cd401fbd0c5bc3603e185a0e
Tree-SHA512: 8773c788d92f2656952e1beac147ba9956b8c5132d474e0880e4c89ff53642928b4cbfcd1cb3d17798b9284f02618a8830c93a9f7a4733e5bded96adff1d5d4d
|
|
772ea4844c34ad70d02fd0bd6c0945baa8fff85c wallet: Avoid recursive lock in IsTrusted (João Barbosa)
819f10f6718659eeeec13af2ce831df3a0984090 wallet, refactor: Immutable CWalletTx::pwallet (João Barbosa)
Pull request description:
This change moves `CWalletTx::IsTrusted` to `CWallet` in order to have TSAN. So now `CWallet::IsTrusted` requires `cs_wallet` and the recursive lock no longer happens.
Motivated by https://github.com/bitcoin/bitcoin/pull/19289/files#r473308226.
ACKs for top commit:
meshcollider:
utACK 772ea4844c34ad70d02fd0bd6c0945baa8fff85c
hebasto:
ACK 772ea4844c34ad70d02fd0bd6c0945baa8fff85c, reviewed and tested on Linux Mint 20 (x86_64).
Tree-SHA512: 702ffd928b2f42a8b90de398790649a5fd04e1ac3877558da928e94cdeb19134883f06c3a73a6826c11c912facf199173375a70200737e164ccaea1bec515b2a
|
|
ecdsa_signature_parse_der_lax(...)
46fcac1e4b9e0b1026bc0b663582148b2fd60390 tests: Add fuzzing harness for ec_seckey_import_der(...) and ec_seckey_export_der(...) (practicalswift)
b667a90389cce7e1bf882f4ac78323c48858efaa tests: Add fuzzing harness for SigHasLowR(...) and ecdsa_signature_parse_der_lax(...) (practicalswift)
Pull request description:
Add fuzzing harness for `SigHasLowR(...)` and `ecdsa_signature_parse_der_lax(...)`.
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 :)
ACKs for top commit:
Crypt-iQ:
ACK 46fcac1e4b9e0b1026bc0b663582148b2fd60390
Tree-SHA512: 11a4856a1efd9a04030a8c8aee2413fd5be1ea248147e649a48a55bacdf732bb48a19ee1ce2761d47d4dd61c9598aec53061b961b319ad824d539dda11a8ccf4
|
|
24bf17602c620445f76c3b407937751c8a894d37 gui refactor: Inline SplashScreen::ConnectWallet (Russell Yanofsky)
e4f435047121886edb6e6a6c4e4998e44ed2e36a refactor: Move wallet methods out of chain.h and node.h (Russell Yanofsky)
b266b3e0bf29d0f3d5deaeec62d57c5025b35525 refactor: Create interfaces earlier during initialization (Russell Yanofsky)
Pull request description:
Add WalletClient interface so node interface is cleaner and don't need wallet-specific methods.
The new NodeContext::wallet_client pointer will also be needed to eliminate global wallet variables like ::vpwallets in #19101, because createWallet(), loadWallet(), getWallets(), etc methods called by the GUI need a way to get a reference to the list of open wallets if it is no longer a global variable.
ACKs for top commit:
promag:
Code review ACK 24bf17602c620445f76c3b407937751c8a894d37.
MarcoFalke:
ACK 24bf17602c620445f76c3b407937751c8a894d37 🐚
Tree-SHA512: a70d3776cd6723093db8912028c50075ec5fa0a48b961cb1a945f922658f5363754f8380dbb8378ed128c8c858913024f8264740905b8121a35c0d63bfaed7cf
|
|
variance of result values
3edc4e34fe2f92e7066c1455f5e42af2fdb43b99 bench: Prevent thread oversubscription (Hennadii Stepanov)
ce3e6a7cb21d1aa455513970846e1f70c01472a4 bench: Allow skip benchmark (Hennadii Stepanov)
Pull request description:
Split out from #18710.
Some results (borrowed from #18710):
ACKs for top commit:
fjahr:
Code review ACK 3edc4e34fe2f92e7066c1455f5e42af2fdb43b99
Tree-SHA512: df7413ec9ea326564a8e8de54752c9d1444ff7de34edb03e1e0c2120fc333e4640767fdbe3e87eab6a7b389a4863c02e22ad2ae0dbf139fad6a9b85e00f563b4
|
