💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3215378037)
Since these lists are tiny, Instead of a set for deduplication, we might as well use a simple O^2 algorithm, iterate from the back and remove whatever appeared in the front (it will likely be even faster than a set). If you decide to do that, I'd appreciate a randomized unit test checking it against a set deduplication.

💬 l0rinc commented on pull request "rpc: followups for 33106":
(https://github.com/bitcoin/bitcoin/pull/33189#issuecomment-3215395119)
> Hope this helps.

Why do you think that exactly? You didn't say anything new, my comment was about making it obvious from the PR's title what it's about (without having to open it) - and not only to ones who already know what 33106 was about.

💬 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.