🤔 tdb3 reviewed a pull request: "test: add functional test for XORed block/undo files (`-blocksxor` option)"
(https://github.com/bitcoin/bitcoin/pull/30657#pullrequestreview-2239353746)
Approach ACK.
Nice additions.
Left a comment and a nit.

💬 tdb3 commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1717724161)
The help for `-blocksxor` mentions `The created XOR-key will be zeros for an existing blocksdir or when `-blocksxor=0` is set`.

After the `verifychain`, could test that a new `xor.dat` is created (consisting of all zeros) when the node is started with no `xor.dat` and with `-blocksxor=0`. Something like:
```python
assert read_xor_key(node=node) == bytes(8)
```
or maybe lift `NUM_XOR_BYTES` out of `read_xor_key` to enable
```python
assert read_xor_key(node=node) == bytes(NUM_XOR_BYTES)
...

💬 tdb3 commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1717654069)
nitty nit: Since this is being moved, could update to use a more descriptive function name (e.g. `xor_bytes`). Feel free to ignore.

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2290369678)
> I'm a little surprised that the effect is quite so small

@gmaxwell I was as well, so I ran the same benchmark with `-reindex` as well and it gave me similar results:

```
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `echo 81508954f1f89f95b6fdca10c3c471cdb6566c80 && \
./src/bitcoind -datadir=/home/user/.bitcoin \
-reindex \
-dbcache=16384 \
-printtoconsole=0 \
-connect=0 \
-stopatheight=820000` | 40765.075 ± 132.452 | 40671.417 | 40858.733 |
...

👍 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