👍 maflcko approved a pull request: "test: add functional test for XORed block/undo files (`-blocksxor` option)"
(https://github.com/bitcoin/bitcoin/pull/30657#pullrequestreview-2239750096)
ACK faa1b9b0e6de7d213699fbdbc2e25a2a81c35cdc

💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1717985952)
nit: Any reason to make this a global function and pass a node to it? Seems clearer to just make it a member function on the node, like `blocks_path()`, no?

💬 paplorinc commented on pull request "coins: Simplify std::move to ternary in `coins.cpp`":
(https://github.com/bitcoin/bitcoin/pull/30656#discussion_r1718016888)
The commens duplicates what the code already stated, but seems there's an agreement that this isn't an improvement, so I'll just close the PR.

✅ paplorinc closed a pull request: "coins: Simplify std::move to ternary in `coins.cpp`"
(https://github.com/bitcoin/bitcoin/pull/30656)

💬 maflcko commented on pull request "wallet: fix blank legacy detection":
(https://github.com/bitcoin/bitcoin/pull/30621#issuecomment-2290751552)
> What compiler? I don't see those warnings.

Probably the known false positives, see https://cirrus-ci.com/task/5429325778911232?logs=ci#L2365 and https://github.com/bitcoin/bitcoin/issues/29359

💬 vasild commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2290758166)
Another aspect to consider wrt wasting bandwidth: if the sender knows the recipient does not have the transaction (e.g. when a wallet sends its newly created transaction), then doing the extra `INV+GETDATA` round-trip is a waste.

💬 paplorinc commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1718035592)
This was specifically requested, but I'll revert, since I liked the spaces: https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594063834

💬 paplorinc commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1718042579)
I considered the actual type to be just noise in these cases, but you seem to have a stronger preference for minimal diff, reverted.

💬 paplorinc commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1718044983)
The rest of the code was using this

✅ paplorinc closed a pull request: "test: Add a few more corner cases to the base58 test suite"
(https://github.com/bitcoin/bitcoin/pull/30035)

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