💬 willcl-ark commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2446546723)
utACK 0de3e96e333090548a43e5e870c4cb8941d6baf1

Based on `git range-diff 38e24df...0de3e96e333090548a43e5e870c4cb8941d6baf1` .

The cmake tidy-up looks good to me, and I examined newly built binaries for probe and semaphore info. I didn't re-run the testing from my previous ACK.

💬 fanquake commented on pull request "cmake: Revamp `FindLibevent` module":
(https://github.com/bitcoin/bitcoin/pull/31181#issuecomment-2446546898)
Concept ACK - we don't have to restrict CMake usage to `vcpkg`, and this will let us make more cleanups.

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822380551)
Found the PR thanks to @hodlinator, this is how I handled that case: https://github.com/bitcoin/bitcoin/pull/30999/files#diff-71badc1cc71ba46244f7841a088251bb294265f4fe9e662c0ad6b15be540eee4R60

We could a test case for the incomplete trailing number (or any other test that seems relevant here): https://github.com/bitcoin/bitcoin/pull/30999/files#diff-718d0d85269ec81595bd9f9181eea3a74b20244b07f14c546e3e07520b2b5f82R81

💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822392320)
Isn't that done here?
https://github.com/bitcoin/bitcoin/blob/ad37073f2e6ab1f39a59109692f84cc85809f53e/src/test/util_string_tests.cpp#L90

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822395900)
It is indeed now, thanks for checking.

💬 Gary19751957 commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#issuecomment-2446696590)
I want whoever you are off of my emails,and away from my crypto and
assets,I do not know you and have given no permissions for any one to
Access my account or any other information being my GitHub repositorys or
other wise.The FBI has been notified and given the names along with a copy
of this email to further this investigation.Gary Rollins

Gary Rollins

On Wed, Oct 30, 2024, 7:15 AM l0rinc ***@***.***> wrote:

> ***@***.**** commented on this pull request.
> -----------------------
...

💬 laanwj commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2446708418)
> I don't think just sending transactions over encrypted connections gives that much extra privacy.

i'm not advicating to send *just transactions*, v2 connections would have blocks+transactions, all of it. Not sending transactions over v1 makes sure that transactions aren't sent over the network in plaintext.

In the long term, if the idea is to phase out P2P v1, i think demotiing it to a blocksonly-protocol makes sense. It holds the network together against eclipse attacks and accidental
...

📝 allanlealluz opened a pull request: "aa"
(https://github.com/bitcoin/bitcoin/pull/31183)
<!--
*** 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
...

✅ fanquake closed a pull request: "aa"
(https://github.com/bitcoin/bitcoin/pull/31183)

📝 fanquake locked a pull request: "aa"
(https://github.com/bitcoin/bitcoin/pull/31183)
<!--
*** 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
...

💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1822430311)
In baac72581b1c99a41ab16a00a3f25fe938eb0610: "This prevents adversarial nodes from proving
our reconciliation set by spamming reconciliation requests": Couldn't we just check the last time the node did a reconciliation request and simply ignore the request?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1822431933)
Just talked to Bruno a bit more about this.

I think it's fair to compare this delayed set mechanism to what i have in the code there.
I like my solution more because:
- its synced across peers (handles better spying across multiple conns i think?)
- simpler code
- more flexibility (doesn't have to be same trickling as adding to sets or fanouting)

Interested in what other people think though.

💬 theStack commented on pull request "key: clear out secret data in `DecodeExtKey`":
(https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2446786655)
@davidgumberg: Good point, that makes a lot of sense. It seems there are more changes needed to make this work though, e.g. `DecodeBase58Check` currently only accepts a `std::vector<unsigned char>` with the default allocator, so one would need to template that function to accept `std::vector<unsigned char, T>` (if that works, didn't try). Also, assigning the `Base58Prefix` causes a similar type mismatch. Might be a potential follow-up if someone is motivated to look into that?

💬 laanwj commented on pull request "ci: Place datadirs for tests under tmpfs ":
(https://github.com/bitcoin/bitcoin/pull/31182#issuecomment-2446807668)
Using tmpfs for temporary test directories can potentially speed up tests a lot, something to be wary for is that it significantly increases memory usage as well (no idea if this is a problem for the current CI).

💬 laanwj commented on pull request "cmake: Revamp `FindLibevent` module":
(https://github.com/bitcoin/bitcoin/pull/31181#issuecomment-2446822174)
Concept ACK

👍 laanwj approved a pull request: "build: have "make test" depend on "make all""
(https://github.com/bitcoin/bitcoin/pull/31015#pullrequestreview-2404602445)
ACK 2957ca9611916efb570d157b9c7a0b188161660d

👋 maflcko's pull request is ready for review: "ci: Place datadirs for tests under tmpfs "
(https://github.com/bitcoin/bitcoin/pull/31182)

💬 maflcko commented on pull request "ci: Place datadirs for tests under tmpfs ":
(https://github.com/bitcoin/bitcoin/pull/31182#issuecomment-2446967574)
> something to be wary for is that it significantly increases memory usage as well (no idea if this is a problem for the current CI).

I'd say the memory increase shouldn't matter, because on modern distros the default temp dir is already a tmpfs. So if memory was an issue, then running the tests normally (outside the CI) would already lead to issue reports. (I haven't seen any, but I may be missing some).

So I think it is fine to enable this by default without an off-switch for now. If the
...

💬 laanwj commented on pull request "ci: Place datadirs for tests under tmpfs ":
(https://github.com/bitcoin/bitcoin/pull/31182#issuecomment-2446990530)
> I'd say the memory increase shouldn't matter, because on modern distros the default temp dir is already a tmpfs. So if memory was an issue, then running the tests normally (outside the CI) would already lead to issue reports. (I haven't seen any, but I may be missing some).

i don't think anyone runs the tests in low-memory/dis conditions 😄 i've had some issues on custom RISC-V hardware in the past, but nothing significant enough to create an issue about. At least the CI with start with a c
...

💬 naumenkogs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1822539819)
in a8d0976de7edf43d3cfea8dff3afc923580d8f84