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.
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
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
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
fa0572d0f3b083b4c8e2e883a66e2b198c6779f1 Pass mempool reference to chainstate constructor (MarcoFalke)
Pull request description:
Next step toward #19556
Instead of relying on the mempool global, each chainstate is given a reference to a mempool to keep up to date with the tip (block connections, disconnections, reorgs, ...)
ACKs for top commit:
promag:
Code review ACK fa0572d0f3b083b4c8e2e883a66e2b198c6779f1.
darosior:
ACK fa0572d0f3b083b4c8e2e883a66e2b198c6779f1
hebasto:
ACK fa0572d0f3b083b4c8e2e883a66e2b198c6779f1, reviewed and tested on Linux Mint 20 (x86_64).
Tree-SHA512: 12184d33ae5797438d03efd012a07ba3e4ffa0d817c7a0877743f3d7a7656fe279280c751554fc035ccd0058166153b6c6c308a98b2d6b13998922617ad95c4c
c4b85ba704a1b5257dc82786a84f676bacb7b027 Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB (Luke Dashjr)
Pull request description:
Fixes a bug introduced in #19614
The LevelDB-specific fdatasync check was only using `AC_SUBST`, which works for Makefiles, but doesn't define anything for C++. Furthermore, the #define is typically 0 or 1, never undefined.
This fixes both issues by defining it and checking its value instead of whether it is merely defined.
Pulled out of #14501 by fanquake's request
ACKs for top commit:
fanquake:
ACK c4b85ba704a1b5257dc82786a84f676bacb7b027 - thanks for catching and fixing my mistake.
laanwj:
Code review ACK c4b85ba704a1b5257dc82786a84f676bacb7b027
Tree-SHA512: 91d5d426ba000b4f3ee7e2315635e24bbb23ceff16269ddf4f65a63d25fc9e9cf94a3b236eed2f8031cc36ddcf78aeb5916efcb244f415943a8a12f907ede8f9
b35e74ba379bdc12ea6d49a45469f0d6aa74cc27 wallet, refactor: Remove duplicate map lookups in GetAddressBalances (João Barbosa)
Pull request description:
Now just one lookup in `balances` instead of three.
ACKs for top commit:
achow101:
ACK b35e74ba379bdc12ea6d49a45469f0d6aa74cc27
theStack:
ACK b35e74ba379bdc12ea6d49a45469f0d6aa74cc27
practicalswift:
ACK b35e74ba379bdc12ea6d49a45469f0d6aa74cc27
Tree-SHA512: a73c1b336406a569e3bb10290618c5950b944db58ed0b05ff202d097684bb3ba3a5942c8d30443960052aa16438c054e2d02977b67aa901cce665c4df0ee5602
This change prepares for upcoming commit "Do not hide compile-time
thread safety warnings" by replacing AssertLockHeld() with
LockAssertion() where needed.
ca185cf5a14b16d61814d7172284bc8efcd28b69 doc: Document differences in bitcoind and bitcoin-qt locale handling (practicalswift)
Pull request description:
Document differences in `bitcoind` and `bitcoin-qt` locale handling.
Since this seems to be the root cause to the locale dependency issues we've seen over the years I thought it was worth documenting :)
Note that 1.) `QLocale` (used by Qt), 2.) C locale (used by locale-sensitive C standard library functions/POSIX functions and some parts of the C++ standard library such as `std::to_string`) and 3.) C++ locale (used by the C++ input/output library) are three separate things. This comment is about the perhaps surprising interference with the C locale (2) that takes place as part of the Qt initialization.
ACKs for top commit:
hebasto:
re-ACK ca185cf5a14b16d61814d7172284bc8efcd28b69
Tree-SHA512: e51c32f3072c506b0029a001d8b108125e1acb4f2b6a48a6be721ddadda9da0ae77a9b39ff33f9d9eebabe2244c1db09e8502e3e7012d7a5d40d98e96da0dc44
8e35bf59062b3a823182588e0bf809b3367c2be0 scripted-diff: rename misbehavior members (John Newbery)
1f96d2e673a78220eebf3bbd15b121c51c4cd97b [net processing] Move misbehavior tracking state to Peer (John Newbery)
7cd4159ac834432dadd60a5e8ee817f3cadbee55 [net processing] Add Peer (John Newbery)
aba03359a6e62a376ae44914f609f82a1556fc89 [net processing] Remove CNodeState.name (John Newbery)
Pull request description:
We currently have two structures for per-peer data:
- `CNode` in net, which should just contain connection layer data (eg socket, send/recv buffers, etc), but currently also contains some application layer data (eg tx/block inventory).
- `CNodeState` in net processing, which contains p2p application layer data, but requires cs_main to be locked for access.
This PR adds a third struct `Peer`, which is for p2p application layer data, and doesn't require cs_main. Eventually all application layer data from `CNode` should be moved to `Peer`, and any data that doesn't strictly require cs_main should be moved from `CNodeState` to `Peer` (probably all of `CNodeState` eventually).
`Peer` objects are stored as shared pointers in a net processing global map `g_peer_map`, which is protected by `g_peer_mutex`. To use a `Peer` object, `g_peer_mutex` is locked, a copy of the shared pointer is taken, and the lock is released. Individual members of `Peer` are protected by different mutexes that guard related data. The lifetime of the `Peer` object is managed by the shared_ptr refcount.
This PR adds the `Peer` object and moves the misbehaving data from `CNodeState` to `Peer`. This allows us to immediately remove 15 `LOCK(cs_main)` instances.
For more motivation see #19398
ACKs for top commit:
laanwj:
Code review ACK 8e35bf59062b3a823182588e0bf809b3367c2be0
troygiorshev:
reACK 8e35bf59062b3a823182588e0bf809b3367c2be0 via `git range-diff master 9510938 8e35bf5`
theuni:
ACK 8e35bf59062b3a823182588e0bf809b3367c2be0.
jonatack:
ACK 8e35bf59062b3a823182588e0bf809b3367c2be0 keeping in mind Cory's comment (https://github.com/bitcoin/bitcoin/pull/19607#discussion_r470173964) for the follow-up
Tree-SHA512: ad84a92b78fb34c9f43813ca3dfbc7282c887d55300ea2ce0994d134da3e0c7dbc44d54380e00b13bb75a57c28857ac3236bea9135467075d78026767a19e4b1
4ec49f8d1e25b330e6a0f79ae897d98d29ff32f9 qt: Leverage the default "Create new receiving address" button (Hennadii Stepanov)
4227a8e1f3a4f94a5a4cb7adeecf967e14156bde qt: Make "Create new receiving address" default unconditionally (Hennadii Stepanov)
Pull request description:
Fix#24
The first commit:
- visual improvement with no behavior change
The second commit:
- removes a bunch of LOCs
- slightly change behavior and makes it standard
With this PR:
ACKs for top commit:
Saibato:
Concept tACK 4227a8e1f34ec49f8d1e
promag:
Tested ACK 4ec49f8d1e25b330e6a0f79ae897d98d29ff32f9 on macos.
Tree-SHA512: 3403d5ee96ec139491c7e23b24a24d9239fe55c58d99cbd4cd13bc877f76f992ed011c09e2af35b2a63be1a2371b95f6ac719325396dcc8333cf3eb7fa2e3d2c
7b6d0f10a7af7998f7cfcf3aeaa0269b61a321ce Remove old check for 3-byte shifted IP addresses from pre-0.2.9 node messages (Raúl Martínez (RME))
Pull request description:
The change removes an old check for IPv6 addresses in range ::ff:ff00:0:0:0/72 that were created due to a bug in size field of addr messages for 0.2.8 nodes and before.
This check is no longer needed as they are no more pre 0.2.9 nodes on the network (as per bitnodes network snapshot).
Credits for discovering this go to sipa in https://github.com/bitcoin/bitcoin/pull/19628#discussion_r475907453
Thanks for the attention!
ACKs for top commit:
sipa:
utACK 7b6d0f10a7af7998f7cfcf3aeaa0269b61a321ce
vasild:
ACK 7b6d0f1
Tree-SHA512: c5fab59dda2acafe143f607a4c5b636a54ac76fba651cad1ad1b09c94e88ab39503a31c2244c8f2664da68456c2a870c601d8894139c55cde9ece8161913ed2e
d3e8adfada889a3c9fba930086eda609509aca07 util: remove c-string interfaces for DecodeBase58{Check} (Sebastian Falbesoner)
Pull request description:
This micro-PR gets rid of base58 function interfaces that are redundant in terms of c-string / std::string variants; the c-string interface for `DecodeBase58Check` is completely unused outside the base58 module, while the c-string interface for `DecodeBase58` is only used in unit tests, where an implicit conversion to std::string is not problematic.
ACKs for top commit:
practicalswift:
ACK d3e8adfada889a3c9fba930086eda609509aca07 -- patch looks correct
laanwj:
Code review ACK d3e8adfada889a3c9fba930086eda609509aca07
Tree-SHA512: 006a4a1e23b11385f60820c188b8e6b1634a182ca36e29a6580f72150214c65a3fdb273ec439165f26ba88a42d2bf5bab1cf3666a9eaee222fb4e1c00aeba433
c91b241b48d7f97b3e6b39d84ec780f2a3e3a0a7 Updated outdated help command for getblocktemplate (fixes#19625) (Jake Leventhal)
Pull request description:
**Summary of Changes**
* Removed coinbasetxn from the help outputs
* Added the missing name for transactions in the help outputs
* Added help outputs for longpollid and default_witness_commitment
* Added more clarity to capabilities, rules, and coinbaseaux
**Rationale**
The outputs from the help command for `getblocktemplate` are outdated and don't reflect the actual results from `getblocktemplate` (see #19625 for more details)
Fixes#19625.
ACKs for top commit:
laanwj:
ACK c91b241b48d7f97b3e6b39d84ec780f2a3e3a0a7
fjahr:
utACK c91b241b48d7f97b3e6b39d84ec780f2a3e3a0a7
Tree-SHA512: ee443af4bc3b2838dfd92e2705f344256ee785ae720e505fffea9b0ec5b75930e3b1374bae59b36d5da57c85c9aefe4d62504b028b893d6f2914dccf1e34c658
The change removes an old check for IPv6 addresses in range ::ff:ff00:0:0:0/72 that were created due to a bug in size field of addr messages for 0.2.8 nodes and before.
This check is no longer needed as they are no more pre 0.2.9 nodes on the network (as per bitnodes network snapshot).
Credits for discovering this go to sipa.
df536883d263781c2abe944afc85f681cda635ed chain: Remove UB CChain comparison (Carl Dong)
Pull request description:
Comparing two empty `CChain`s is currently undefined behaviour, and resulted in false assertion failures when comparing identical empty `CChain`s in local testing.
Let's just remove this comparison operator since it doesn't seem to be used anywhere.
ACKs for top commit:
practicalswift:
ACK df536883d263781c2abe944afc85f681cda635ed -- patch is guaranteed to be correct :)
MarcoFalke:
cr ACK df536883d263781c2abe944afc85f681cda635ed
Tree-SHA512: db10bac364fc965b56abf7a5bac48018786b14806ffe107e3e8eb24d5004a29331f3387dfe3409a3452a6750d3329e3f354265d787ebb3abfccabe77b28a54d5
cc26fab48d76a813d798657b18ae1af08a301150 tests: Add fuzzing harness for CNode (practicalswift)
Pull request description:
Add fuzzing harness for `CNode`.
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: e6330e5de5b2eb44d3bd91a885e69ebb625bfd1cb2499338aeb3997ff0268848434e651126fe68a8cadd7235c391e61a40d6408ee26e457faf73572e0c375f6b
b6dcc6d74186eee15eda2cb6e8a7ab5b5b4a05f8 gui: Clarify block height label (Hennadii Stepanov)
Pull request description:
Prefer "block height" instead of "number of blocks".
This was done while testing https://github.com/bitcoin/bitcoin/pull/16981.
ACKs for top commit:
michaelfolkson:
ACK b6dcc6d74186eee15eda2cb6e8a7ab5b5b4a05f8. I don't think there are any other obvious examples in the GUI where "block height" should replace "number of blocks" except for translations.
MarcoFalke:
cr ACK b6dcc6d74186eee15eda2cb6e8a7ab5b5b4a05f8
Tree-SHA512: ec3b48c1af5d613ed657ad51f2caddea774376736ecc02343d54518986e35ec37f1745b059814b5be92b5e5c2bb2970d17159b24c6e88b9316803d4de5327c31
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, 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.
Also tweaks splash screen registration for load wallet events to be delayed
until after wallet client is created.
Add AppInitInterfaces function so wallet chain and chain client interfaces are
created earlier during initialization. This is needed in the next commit to
allow the gui splash screen to be able to register for wallet events through a
dedicated WalletClient interface instead managing wallets indirectly through
the Node interface. This only works if the wallet client interface is created
before the splash screen needs to use it.
a99a3c0bd6d5476503c015e23be569295fdd190c rpc: Validate provided keys for query_options parameter in listunspent (pasta)
Pull request description:
At Dash, one of our developers was working with the `listunspent` RPC command, but instead of saying "minimumAmount" he said "minimmumAmount" as such the RPC wasn't working as expected.
In https://github.com/dashpay/dash/pull/3507 we implemented a check so that `listunspent` returns an error if an unrecognized option is given. I figured I might as well adapt the code and throw up a PR here.
Cheers!
ACKs for top commit:
adaminsky:
ACK `a99a3c0bd`
meshcollider:
Seems fine to me. utACK a99a3c0bd6d5476503c015e23be569295fdd190c
Tree-SHA512: 9fccf14979849879a51b352afa3e1932ce4a6cfc2ee97b8d405ec6e65673fe94e302795e3ec0b440e6d252f13acda620e1f6a0e86c3fa918883c3fb4600a372c
This is needed to allow bitcoin-gui to connect to existing node process with
-ipcconnect instead of spawning a new process. It's possible to spawn a new
bitcoin-node process without knowing the current data dir or network, but
connecting to an existing bitcoin-node requires knowing the datadir and network
first.
No change in behavior. Replacing references with pointers allows Node interface
creation to be delayed until later during gui startup next commit to support
implementing -ipcconnect option
Change gui code to use gArgs, Params() functions directly instead of going
through interfaces::Node.
Remotely accessing bitcoin-node ArgsManager from bitcoin-gui works fine in
https://github.com/bitcoin/bitcoin/pull/10102, when bitcoin-gui spawns a new
bitcoin-node process and controls its startup, but for bitcoin-gui to support
-ipcconnect option in https://github.com/bitcoin/bitcoin/pull/19461 and connect
to an existing bitcoin-node process, it needs ability to parse arguments itself
before connecting out.
This change also simplifies https://github.com/bitcoin/bitcoin/pull/10102 a
bit, by making the bitcoin-gui -> bitcoin-node startup sequence more similar to
the bitcoin-node -> bitcoin-wallet startup sequence where the parent process
parses arguments and passes them to the child process instead of the parent
process using the child process to parse arguments.
e2aa1a585a83971639572cd2c84565ec360deac9 util: make EncodeBase64 consume Spans (Sebastian Falbesoner)
2bc207190e6f1818b04c9336f6e0d625b2a2a0ba util: make EncodeBase32 consume Spans (Sebastian Falbesoner)
Pull request description:
To simplify the interface of the Base32/Base64 encoding functions for raw data, this PR changes them from taking two arguments (pointer and length) to just one Span. Most calls to `EncodeBase64` pass data from `CDataStream` instances, which unfortunately internally work with `char*` pointers rather than `unsigned char*`, but thanks to the recently introduced `MakeUCharSpan` helper, converting them is quite easy.
ACKs for top commit:
MarcoFalke:
ACK e2aa1a585a83971639572cd2c84565ec360deac9 🐮
vasild:
ACK e2aa1a585
Tree-SHA512: 43bd3bd2ee8e3be2474db0a81dae9d9e88fac2464b96d1b042147106ed7433799dcba3000c69990511ecfc697b0c7306ce85f2ecb2293e2e44fd356c9694b150
fa9d5902f7d72e8cce105dd1b1f5a1062e304b10 scripted-diff: gArgs -> args (MarcoFalke)
fa33bc2dabbbd2d73961f9b0ce51420a3b6e4ad5 init: Capture copy of blocknotify setting for BlockNotifyCallback (MarcoFalke)
fa40017706e08b4de111e8e57aabeced60881a57 init: Pass reference to ArgsManager around instead of relying on global (MarcoFalke)
Pull request description:
The gArgs global has several issues:
* gArgs is used by each process (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-cli, bitcoin-tx, ...), but it is hard to determine which arguments are actually used by each process. For example arguments that have never been registered, but are still used, will always return the fallback value.
* Tests may run several sub-tests, which need different settings. So globals will have to be overwritten, but that is fragile on its own: e.g. https://github.com/bitcoin/bitcoin/pull/19704#issuecomment-678259092 or #19511
The goal is to remove gArgs, but as a first step in that direction this pull will change gArgs in init to use a passed-in reference instead.
ACKs for top commit:
ryanofsky:
Code review ACK fa9d5902f7d72e8cce105dd1b1f5a1062e304b10. Looks good. Nice day to remove some globals, and add some lambdas 👍
fanquake:
ACK fa9d5902f7d72e8cce105dd1b1f5a1062e304b10 - I'm not as familiar with the settings & argument handling code, but this make sense, and is a step in the right direction towards a reduction in the usage of globals. Not a huge fan of the clang-formatting in the scripted diff.
jonasschnelli:
Concept ACK fa9d5902f7d72e8cce105dd1b1f5a1062e304b10
Tree-SHA512: ed00db5f826566c7e3b4d0b3d2ee0fc1a49a6e748e04e5c93bdd694ac7da5598749e73937047d5fce86150d764a067d2ca344ba4ae3eb2704cc5c4fa0d20940f
fad84b7e14ff92465bc17bfdaf1362bcffe092f6 test: Activate segwit in TestChain100Setup (MarcoFalke)
fa11ff29803ca4f5fd0035bede697448cff7d960 test: Pass empty tx pool to block assembler (MarcoFalke)
fa96574b0d2d2c0880447f163cd0280fb3551910 test: Move doxygen comment to header (MarcoFalke)
Pull request description:
This fixes not only a TODO in the code, but also prevents a never ending source of uninitialized reads. E.g.
* #18376
* https://github.com/bitcoin/bitcoin/pull/19704#issuecomment-678259092
* ...
ACKs for top commit:
jnewbery:
utACK fad84b7e14ff92465bc17bfdaf1362bcffe092f6
Tree-SHA512: 64cf16a59656d49e022b603f3b06441ceae35a33a4253b4382bc8a89a56e08ad5412c8fa734d0fc7b58586f40ea6d57b348a3b4838bc6890a41ae2ec3902e378
