💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2046945113)
As mentioned in https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1558318881, with C++20, we can use `__VA_OPT__` and drop the changes from https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1155488768. Done that and rebased.

👍 rkrux approved a pull request: "test: Makes `wait_for_getdata` delete data on checks, plus allows to check the getdata message type"
(https://github.com/bitcoin/bitcoin/pull/29748#pullrequestreview-1991162251)
tACK [067b009](https://github.com/bitcoin/bitcoin/pull/29748/commits/067b009bbf47f7bc5adeb6b500042f7c44bfb03f)

Make successful, functional tests successful

I believe this is a good refactor introducing `check_last_getdata` and modifying `wait_for_getdata` that will reduce some level of code duplication within tests.

Regarding this comment, I tried this refactor and the tests pass.
> check_last_getdata uses the exact same logic as the internal function for wait_for_getdata. It feels lik
...

💬 maflcko commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1559107527)
question, feel free to ignore:

Does it need to be in a separate macro, to be put in the global namespace, or would it work to use `static` inside the function scope?

If so, it could be combined into the `TRACEPOINT` macro, and using `TRACEPOINT` without it would not be possible.

✅ ajtowns closed a pull request: "policy: Allow non-standard scripts with -acceptnonstdtxn=1"
(https://github.com/bitcoin/bitcoin/pull/28334)

📝 ajtowns opened a pull request: "policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)"
(https://github.com/bitcoin/bitcoin/pull/29843)
This PR changes `-acceptnonstdtxn=1` so that it also skips the `non-mandatory` script checks, allowing txs that (eg) use OP_NOPx or OP_SUCCESS into the mempool. This remains only available on test nets.

💬 ajtowns commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)":
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2046987090)
Previous discussion in #28334.

If you enable this feature as a non-mining node, you risk having txs pinned in your mempool that conflict or double-spend standard txs in the normal mempool, which may cause problems with applications or layer-2 protocols.

If you enable this feature as a mining node, you have a small risk of ending up with invalid txs remaining in your mempool if a tx non-standard tx is accepted in your mempool and remains in there during a soft-fork activation that renders t
...

💬 ajtowns commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1":
(https://github.com/bitcoin/bitcoin/pull/28334#issuecomment-2046988860)
Closed and re-opened fresh as #29843

💬 fanquake commented on pull request "ci: disable `_FORTIFY_SOURCE` with MSAN":
(https://github.com/bitcoin/bitcoin/pull/29837#issuecomment-2047043268)
Tested this over all 4 jobs. OSS-Fuzz PR is here: https://github.com/google/oss-fuzz/pull/11792.

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