💬 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

⚠️ southunitedraza opened an issue: "Tokensg"
(https://github.com/bitcoin/bitcoin/issues/31185)

✅ fanquake closed an issue: "Tokensg"
(https://github.com/bitcoin/bitcoin/issues/31185)

:lock: fanquake locked an issue: "Tokensg"
(https://github.com/bitcoin/bitcoin/issues/31185)

💬 andrewtoth commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-2447160934)
> this approach is possible but is it maintainable?

Hmm, yeah this would end up with `reserve(<magic number>)` littered throughout the json serialization. Not ideal.

I took a quick look through most of the block parsing, and many of the reservations we would do would be <5, so unlikely to be a big benefit.

The `entry` in `TxToUniv` could have up to 12 kv-pairs stored, and in `blockheaderToJSON` it could have 14. These would also be reserving two vectors so might have some visible benefi
...

💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#issuecomment-2447182783)
Rebased #30933 on top of this PR and the mismatches between our custom `consteval` checking and tinyformat are gone as far as our current tests go - rebased commit: 32d4cf37d5056dc42bbf317b059e10910b984b0e

💬 maflcko commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#issuecomment-2447191411)
Are you still working on this? If not, maybe turn into a draft for now?

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#issuecomment-2447224880)
re: https://github.com/bitcoin/bitcoin/pull/31174#issuecomment-2447182783

> mismatches between our custom `consteval` checking and tinyformat are gone as far as our current tests go

In case you do want a test with different behavior, I think you can use %n specifier which is not supported by tinyformat

- rebased commit: [32d4cf3](https://github.com/bitcoin/bitcoin/commit/32d4cf37d5056dc42bbf317b059e10910b984b0e)

💬 maflcko commented on pull request "ci: Place datadirs for tests under tmpfs ":
(https://github.com/bitcoin/bitcoin/pull/31182#issuecomment-2447229422)
(force pushed to fix a type bug in the modified Python code, discovered by the Windows CI. I wish this code was written in Rust or another type-safe language, so that issues like this are caught at compile-time, but this can be done in a follow-up)

✅ maflcko closed a pull request: "doc: note that you can assume C++20."
(https://github.com/bitcoin/bitcoin/pull/30136)

💬 maflcko commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#issuecomment-2447301242)
Closing for now, due to inactivity since May. (The feedback was waiting to be addressed since then)

Please leave a comment if you want this reopened, or you can open a new pull request with the new changes.

💬 davidgumberg commented on pull request "key: clear out secret data in `DecodeExtKey`":
(https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2447431280)
utACK https://github.com/bitcoin/bitcoin/pull/31166/commits/559a8dd9c0aafcecf00f9ccd9aabe5720bcebe8c

I agree that given how short lived this data is it doesn't seem likely to get paged, but it would still be nice to encapsulate our allocation strategy for secrets.

💬 theStack commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#issuecomment-2447431668)
Concept ACK