💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy check and error":
(https://github.com/bitcoin/bitcoin/pull/33082#issuecomment-3215466979)
<ins>_Updates_</ins>
- Addressed @brunoerg's [feedback](https://github.com/bitcoin/bitcoin/pull/33082#discussion_r2291906741); removed redundant comment, as the added assertion makes the message clear enough.

💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294732188)
Yeah, I don't mind the order much, especially now that it seems like neither will be in v30. Thanks for asking!

💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294736486)
Oh, so `clang-format` was pushing for it... seems it could do with some reconfiguration in the initializer-list aspect.

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294742258)
After https://github.com/bitcoin/bitcoin/pull/32813 we could customize the list formatting as well

👍 TheCharlatan approved a pull request: "threading: remove ancient CRITICAL_SECTION macros"
(https://github.com/bitcoin/bitcoin/pull/32592#pullrequestreview-3146111612)
ACK 46ca7712cb5fcf759cfc9f4f32d74215c8c83763

💬 TheCharlatan commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#discussion_r2294770722)
Not really related to this PR, but I really don't like how long the scope of the lock is here. We could reduce this a bit by doing something like
```diff
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index b710c605bc..750ca72bb4 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -707,2 +706,0 @@ static RPCHelpMan getblocktemplate()
- WAIT_LOCK(cs_main, csmain_lock);
- uint256 tip{CHECK_NONFATAL(miner.getTip()).value().hash};
@@ -737,0 +736 @@ static RPCHelpMan getb
...

📝 ryanofsky opened a pull request: " Update libmultiprocess subtree to fix build issues"
(https://github.com/bitcoin/bitcoin/pull/33241)
Includes:

- https://github.com/bitcoin-core/libmultiprocess/pull/193
- https://github.com/bitcoin-core/libmultiprocess/pull/195
- https://github.com/bitcoin-core/libmultiprocess/pull/194

These changes are needed to build fix libmultiprocess build issue that happens on OpenBSD and work around an incompatibility between GCC versions <14 and cap'nproto versions <0.9 when compiling with c++20 that was fixed upstream in https://github.com/capnproto/capnproto/pull/1170. The issues were report
...

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2294922802)
Negatives values would make the code crash because of the Assume introduced in `GetFee` https://github.com/bitcoin/bitcoin/blob/73220fc0f958f9b65f66cf0cf042af220b312fc6/src/policy/feerate.cpp#L22

```
$ FUZZ=wallet_fees build_fuzz/bin/fuzz
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2952456953
INFO: Loaded 1 modules (614477 inline 8-bit counters): 614477 [0x5832503883e0, 0x58325041e42d),
INFO: Loaded 1 PC tables (614477 PCs): 614477 [0x58325041e430,0x583250d7e90
...

💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2294992281)
This is done in: https://github.com/bitcoin/bitcoin/pull/33082/commits/30c6f64eed304560464f9601b80c811c186db20a

💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2294992891)
Nice find, sorry I missed this.

💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2295019319)
This was fixed in https://github.com/bitcoin/bitcoin/pull/32481/commits/5431f2dc2159f55e0fbe89d07deb97fe2a73fb43 (nice!), fixed on this branch by rebasing.

💬 nervana21 commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#issuecomment-3216136406)
concept ACK

💬 kannapoix commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#issuecomment-3216227775)
ACK aabf1f6093892d372544c8b5d55d260699dab69c

nit: It may be a good idea to also update the example command in the help message of getblockstats. I think it would be better to use plain text as an argument instead of the one surrounded with quotes.
https://github.com/bitcoin/bitcoin/blob/73220fc0f958f9b65f66cf0cf042af220b312fc6/src/rpc/blockchain.cpp#L1943

💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2295121329)
Thanks for pointing this out, I've updated the PR description to address this.

💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2295237186)
Thanks for pointing this out, I've fixed this issue in https://github.com/bitcoin/bitcoin/pull/32636/commits/2826ccd38a56ef6f6a6dde0830c5e0ad3bd40ebb

💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2295238189)
Thanks for the suggestion, fixed in https://github.com/bitcoin/bitcoin/pull/32636/commits/b5de137824d266357dd34953a4dc506289acf83c

💬 maflcko commented on pull request "Update libmultiprocess subtree to fix build issues":
(https://github.com/bitcoin/bitcoin/pull/33241#issuecomment-3216370455)
> Task `ARM, unit tests, no functional tests`: https://github.com/bitcoin/bitcoin/runs/48709103843
> LLM reason (✨ experimental): The CI failure is caused by a test timeout during the execution of the 'mptest' test.

Unrelated, but this failure looks real? The unit test should normally pass in a few milliseconds, so taking 40 minutes seems odd?

https://cirrus-ci.com/task/5714850606743552?logs=ci#L2822: `[23:07:33.279] 3/148 Test #3: mptest ............................... Passed 0.
...

💬 waketraindev commented on pull request "rpc: add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2295255521)
Added to changeset

💬 waketraindev commented on pull request "rpc: add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2295273936)
RPCArg doesn't support '|' in names and will throw a rpc error when it gets called. https://github.com/bitcoin/bitcoin/blob/73220fc0f958f9b65f66cf0cf042af220b312fc6/src/rpc/util.cpp#L923

💬 davidgumberg commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#issuecomment-3216443821)
@achow101
> `PopulateWalletFromDB` inside of `CreateNew` should no longer be necessary.

@pablomartin4btc
> DBErrors WalletBatch::LoadWallet(CWallet* pwallet) is being called during wallet creation I think.

Thanks for catching this, fixed in https://github.com/bitcoin/bitcoin/pull/32636/commits/acc0d3b7d4e8b26e1dc5832497b2b47047b6a453.

> It'd be nice if the load or create intention could be propagated down to the DB level as well. In `SQLiteDatabase::Open`, we pass the flags `SQLITE
...