💬 laanwj commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-2047055895)
Right, there's something to be said to keep that where it already snuck in, to not break the existing interface. And i guess if the other enumeration items already are parsed case-insensitively, doing that for "UNSET" makes sense too, otherwise it's silly.

i just think the consensus is that this is not a direction the RPC interface is moving in in general, and i think introducing case-insensitive API was a mistake. But maybe getting rid of this has too much breaking impact now, I don't know.

💬 laanwj commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2047058991)
Concept ACK

💬 brunoerg commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1559169522)
In dfd635bcc9b3f3615cabe115af302df33efba6a1 "net: Make AddrFetch connections to fixed seeds": When running multiple networks, wouldn't it be good to ensure that we're covering all networks? I mean, having at least one per network of the 10 ones.

👍 fanquake approved a pull request: "Replace Boost.Process with cpp-subprocess"
(https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752)
ACK d5a715536e497c160a2520f81334aab6c7490213 - with the expectation that this code is going to be maintained as our own. Next PRs should:
* Remove linter exclusions and fix all issues.
* Delete all unused code.
* Rewrite in C++20.
* hpp -> cpp ?

💬 maflcko commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-2047093492)
Does it work with `OBJ_NAMED_PARAMS`?

💬 dergoegge commented on pull request "minisketch: pull subtree + #81":
(https://github.com/bitcoin/bitcoin/pull/29823#issuecomment-2047098733)
Fuzzed for 2000 CPU hours, no more UB was found

✅ fanquake closed an issue: "RFC: Replacing Boost Process"
(https://github.com/bitcoin/bitcoin/issues/24907)

🚀 fanquake merged a pull request: "Replace Boost.Process with cpp-subprocess"
(https://github.com/bitcoin/bitcoin/pull/28981)

👍 dergoegge approved a pull request: "minisketch: pull subtree + #81"
(https://github.com/bitcoin/bitcoin/pull/29823#pullrequestreview-1991284585)
ACK c03fd4e58bfd1ce7f4e67283e64152690cf25c21

💬 brunoerg commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1559180575)
> I think that usually it shouldn't substantially change the observations of a user, because 10 is a pretty high number and if all of those fail, we either have rally bad fixed seeds, or have a connectivity problem.

I agree. I've tested it following `contrib/seeds/README.md` to generate the seeds and the worst scenario was connecting with 5 of the 10.

💬 maflcko commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-2047125355)
Looks like this also fixed the Windows issue (https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-1523507720), so I guess there may have been a bug in the previous implementation.

💬 maflcko commented on pull request "doc: update NeedsRedownload() and nStatus comment":
(https://github.com/bitcoin/bitcoin/pull/29624#issuecomment-2047139574)
lgtm ACK 6cfac2c9094cdb2cc35efa72adef60063693fbfe

I did not test this, to confirm the error message.

📝 theStack opened a pull request: "depends: remove no longer needed patch for Boost::Process"
(https://github.com/bitcoin/bitcoin/pull/29844)
As Boost::Process has been replaced by cpp-subprocess (PR #28981), this patch touches an unused code part and is hence not needed anymore.

👍 rkrux approved a pull request: "test: Refactor fee calculation to remove satoshi_round function"
(https://github.com/bitcoin/bitcoin/pull/29566#pullrequestreview-1991321508)
tACK [b7a4a81](https://github.com/bitcoin/bitcoin/pull/29566/commits/b7a4a8118b43ebd2ab1b64247990ff49d6f19d26)

Make successful, all functional tests successful.

> To address this, the satoshi_round function is removed, and Decimal values are utilized in its place.
1. As per the second commit, the `satoshi_round` is back, let's update the PR description to reflect that because it will be picked up by the bot on merge?
2. Should we consider squashing the first 2 commits into one because af
...

💬 rkrux commented on pull request "test: Refactor fee calculation to remove satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1559202803)
We can't use `satoshi_round()` here because it will by default round down and that's not what's needed here? Wondering if that function can also provide an option to not round at all.

👍 hebasto approved a pull request: "depends: remove no longer needed patch for Boost::Process"
(https://github.com/bitcoin/bitcoin/pull/29844#pullrequestreview-1991326457)
ACK 95c594f4e9ea3cb57aa03b75d4d70fe0e1742065, I have reviewed the code and it looks OK.

💬 sr-gi commented on pull request "test: Extends wait_for_getheaders so a specific block hash can be checked":
(https://github.com/bitcoin/bitcoin/pull/29736#discussion_r1559208410)
Sure, because "getheaders" is in `last_message` for that peer, but only for that one as the assert after the loop is checking.

The later `wait_for_getheaders` (the one on `expected_peer`) is checked on the other node only. I guess that is not too clear based on how `expected_peer` is initialized. We could just set `expected_peer` as follows to make it clearer:

```python
for p in [peer2, peer3]:
with p2p_lock:
if "getheaders" in p.last_message:
count += 1

...

👍 fanquake approved a pull request: "depends: remove no longer needed patch for Boost::Process"
(https://github.com/bitcoin/bitcoin/pull/29844#pullrequestreview-1991335394)
ACK 95c594f4e9ea3cb57aa03b75d4d70fe0e1742065

📝 stickies-v opened a pull request: "rpc: return warnings as an array instead of just a single one"
(https://github.com/bitcoin/bitcoin/pull/29845)
The RPC documentation for `getblockchaininfo`, `getmininginfo` and `getnetworkinfo` states that "warnings" returns "any network and blockchain warnings". In practice, only a single warning is returned.

Fix that by returning all warnings as an array.

As a side benefit, clean up the GetWarnings() logic.

Since this PR changes the RPC result schema, I've added release notes. Adding a `-deprecatedrpc` option for a change this simple seems overkill in my view and I'd rather not do, but I'm ha
...

🚀 fanquake merged a pull request: "depends: remove no longer needed patch for Boost::Process"
(https://github.com/bitcoin/bitcoin/pull/29844)