💬 achow101 commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2294520257)
This is here because the test fails without it.

💬 yancyribbens commented on pull request "coinselection: Tiebreak SRD eviction by weight":
(https://github.com/bitcoin/bitcoin/pull/33223#issuecomment-3215406859)
Any reason this is in Draft?

💬 achow101 commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#issuecomment-3215421738)
> I guess this basically works for the existing params, since a block hash is never valid JSON (which forbids leading zeros), but could be dangerous to use for new parameters that are ambiguous.

I don't think we have any types where that would be the case. But this is also something that needs to be turned on for a parameter explicitly, so I don't expect this to be problematic.

> With that in mind, it might be worth instead having a new `RPCResult::Type::BLOCK_HEIGHT_OR_HASH` type to sanit
...

💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy check and error":
(https://github.com/bitcoin/bitcoin/pull/33082#discussion_r2294577799)
Done.

💬 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