💬 paplorinc commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#issuecomment-2290783811)
> I don't think this change makes sense

k, closing.

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718048796)
In other cases we're adding and subtracting booleans, but I'm fine with both, see: https://github.com/bitcoin/bitcoin/pull/30535/files#diff-09e6cf871236bf03d32cca9405837d9b7927690b2296a2de17c9be6ea0e75959R74

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718060639)
That's some next level terseness. Okay, will change if I re-touch.

💬 maflcko commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1718065776)
Seems fine to just add the new data, no?

💬 TheCharlatan commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#issuecomment-2290822734)
I would have ACKed this if it were cleaned up a bit. The new test data and better error messages in the randomized tests are good changes.

👍 TheCharlatan approved a pull request: "optimization: reserve memory allocation for transaction inputs/outputs"
(https://github.com/bitcoin/bitcoin/pull/30093#pullrequestreview-2239919017)
ACK ec585f11c38bbe277a596dcea3c813e7c6626050

💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2290858777)
I realized that I limited the search space too much for the fuzz target that deals with invalid inputs only. I've fixed it in the last push, and added a commit to remove an unused symbol.

This brings back the performance increase to 10x (or so) for the "fast" fuzz target. Also, the bug from https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2285605841 is no longer found "immediately".

One of the processes just found it after 700k iterations after one hour, which seems reasonable.
...

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718117883)
Correct, the issue is not the clang version, but the stdlib version. `MacOSX14.0.sdk` does seem to have an issue in the iterators implementation. The minimum clang (and libc++) version is documented in `doc/dependencies.md` to be clang-16.

However, Apple somehow ships a completely separately versioned clang and stdlib with Xcode. Apples Xcode 14 stdlib seems to be no longer supported after this commit.

Given that Xcode 15 dropped support for macOS Ventura 13, made me leave the previous com
...

💬 fjahr commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2290939844)
I have opened a PR to revert my change to the BIP here: https://github.com/bitcoin/bips/pull/1660

💬 fjahr commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2290942734)
tACK 16e95bda86302af20cfb314a2c0252256d01f750

I was able to sync with this as well (as of yesterday).

✅ hebasto closed a pull request: "test: Fix test log file name"
(https://github.com/bitcoin/bitcoin/pull/30654)

💬 fjahr commented on pull request "doc: mention bip94 support":
(https://github.com/bitcoin/bitcoin/pull/30655#issuecomment-2290950882)
ACK 99eeb51bf65fa00ee863f32d70f478bb9db0e351

🚀 glozow merged a pull request: "doc: mention bip94 support"
(https://github.com/bitcoin/bitcoin/pull/30655)

👍 TheCharlatan approved a pull request: "depends: Amend handling flags environment variables"
(https://github.com/bitcoin/bitcoin/pull/30477#pullrequestreview-2240096761)
ACK d438b501f859befa4f758efcff463c0d647bc033

The important change here is that when used with cmake, the user-defined flags currently get placed in the toolchain file like this (running `make CXXFLAGS="-O1" `):
```
32 if(NOT DEFINED CMAKE_CXX_FLAGS_INIT)
33 set(CMAKE_CXX_FLAGS_INIT "")
34 set(CMAKE_OBJCXX_FLAGS_INIT "")
35 endif()
36 if(NOT DEFINED CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT)
37 set(CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT "-O1")
38 set(CMAKE_OBJCXX_FLAGS_RELWITHDEB
...

🤔 maflcko reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2239975805)
Left some style nits, feel free to ignore.

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718187725)
style nit in https://github.com/bitcoin/bitcoin/commit/b2fbe40cdf08b56d98e38ce261bfeeaf23b5f795:

I wonder if it makes sense to pick the default as `Byte = std::byte` now. Obviously the diff will be larger, but longer term it seems cleaner to converge to `std::byte`, so having that as default will make the future easier, no?

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718137078)
style nit in 9c4aad3fe4cb74bb1d9afeece78e2dd6ae1ad08e: When renaming this, I'd suggest to use the "fully" correct naming style `ciphertext` (snake_case without type prefix), according to the style guide.

(An alternative would be to leave it completely untouched)

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718210663)
style nit in 0a82c18457ec81e911b835b2ac76ad7475384983:

I know this has been mentioned before, but performance doesn't matter here, so using just `VecFromHex` seems shorter and clearer?

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718211451)
style nit in https://github.com/bitcoin/bitcoin/commit/0a82c18457ec81e911b835b2ac76ad7475384983:

Span and array serialization is the same, so in theory you could drop this call to Span.

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718214663)
style nit in https://github.com/bitcoin/bitcoin/commit/0a82c18457ec81e911b835b2ac76ad7475384983:

Still not sure about this. The benefit seems limited to offer two functions that effectively do the same in tests. It is just extra stuff for test writers (and readers) to keep in mind, when reviewing or writing new tests. Just having one function (the already existing one) seems easier.

The benefit of compile-time enforcement in tests seems limited, because running this test is way faster than
...