💬 maflcko commented on pull request "fuzz: use std::source_location in fuzz/rpc.cpp":
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046766022)
-0 on this refactor. The existing code seems easier to read and is shorter?

Also, albeit not an issue here, when used as default argument the location is wrong, see https://github.com/llvm/llvm-project/issues/56379.

💬 fanquake commented on pull request "fuzz: use std::source_location in fuzz/rpc.cpp":
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046772255)
> The existing code seems easier to read and is shorter?

Yea, I assumed this might be one pushback. I guess the project can decide if this is actually something we want to use in general. Or, if the tradeoff in verbosity is only worth it when used more globally.

✅ fanquake closed a pull request: "fuzz: use std::source_location in fuzz/rpc.cpp"
(https://github.com/bitcoin/bitcoin/pull/29841)

💬 fanquake commented on pull request "fuzz: use std::source_location in fuzz/rpc.cpp":
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046775039)
Actually, will just come back to this after Clang 16.

💬 maflcko commented on pull request "fuzz: use std::source_location in fuzz/rpc.cpp":
(https://github.com/bitcoin/bitcoin/pull/29841#issuecomment-2046784120)
The only reasonable places to use this would be where it is used as a default argument, no? So that'd be:

* https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1850520824
* https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445230410

Ref #29077 for the fix.

📝 Thales-de-Milet opened a pull request: "Create docker-image.yml"
(https://github.com/bitcoin-core/gui/pull/816)


<!--
*** seed:0708ce02816c430ab90bebbf40828693f7f5d111ba0f2b169e5b83213f0200257bd3baa4a2a47b7d115db23aa6de960d48796a3bb1565735a6d349e92abdca4e ***

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

GUI-related pull requests should be opened against
https://github.com/Thales-de-Miles/Bitcoin/gui
first. See CONTRIBUTING.md
-->

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

✅ fanquake closed a pull request: "Create docker-image.yml"
(https://github.com/bitcoin-core/gui/pull/816)

💬 0xB10C commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2046870296)
I've been playing around with bencher running the `bitcoin_bench` bitcoind binary in a GH action as PoC. A sample dashboard is [here](https://bencher.dev/console/projects/0xb10c-s-bitcoin-core/perf?key=true&reports_per_page=4&branches_per_page=8&testbeds_per_page=8&benchmarks_per_page=8&reports_page=1&branches_page=1&testbeds_page=1&benchmarks_page=1&report=915c1007-33dd-4d30-b9c1-531b60af8a06&branches=0ab6caf7-716b-4a6e-8101-f2b9c891a9a2&testbeds=3c0c4d9d-b40e-4d9a-a462-2379355b0785&benchmarks=
...

💬 real-or-random commented on pull request "Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305":
(https://github.com/bitcoin/bitcoin/pull/28263#issuecomment-2046884446)
@theStack want to re-review this? :)

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