💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1559054301)
Going over this PR again, shouldn't this have been a separate `if` statement instead of an `if/else` (or not a condition at all)?

Right now we are accepting expressions of the type `@ip:port` (which where already accepted before), but not expressions of the type `in/out@ip:port`.

The former will pass and default to `ConnectionDirection::In + NetPermissionFlags::Implicit`, while the latter will be rejected. That feels inconsistent

💬 ajtowns commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1":
(https://github.com/bitcoin/bitcoin/pull/28334#issuecomment-2046913270)
Note that the concept aks in the drahtbot report were mostly/entirely for the "don't do this on testnet by default" aspect of this PR which was split out into #28354.

💬 brunoerg commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1559069735)
In "net: Make AddrFetch connections to fixed seeds" dfd635bcc9b3f3615cabe115af302df33efba6a1: Perhaps it would be interesting to specify in the log it will initiate 10 AddrFetch connections instead of just saying "to fixed seeds".

📝 loselarry opened a pull request: "doc: fix some typos"
(https://github.com/bitcoin/bitcoin/pull/29842)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 maflcko commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1":
(https://github.com/bitcoin/bitcoin/pull/28334#issuecomment-2046936190)
Maybe reopen as a fresh pull request, if still relevant, so that there is a clean and easy to follow discussion?

✅ fanquake closed a pull request: "doc: fix some typos"
(https://github.com/bitcoin/bitcoin/pull/29842)

💬 fanquake commented on pull request "doc: fix some typos":
(https://github.com/bitcoin/bitcoin/pull/29842#issuecomment-2046937716)
Please send changes to our upstream subtree repositorys. See the notes on subtrees in the [developer-notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees).

💬 fanquake commented on issue "build: `CPPFLAGS` usage in OSS-Fuzz":
(https://github.com/bitcoin/bitcoin/issues/29385#issuecomment-2046940708)
Should be closed after https://github.com/google/oss-fuzz/pull/11725.

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