💬 brunoerg commented on pull request "coins: Simplify std::move to ternary in `coins.cpp`":
(https://github.com/bitcoin/bitcoin/pull/30656#discussion_r1717638941)
@TheCharlatan +1

Also, the current code is clearer for me, especially due to the comment.

💬 ajtowns commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2290151775)
> You also mentioned wasting inbound slots, but requiring `INV+GETDATA` will not solve that - the mass connect tool will be updated to do `INV+GETDATA+TX` instead of just `TX`. Those are short-lived connections anyway.

Sending INV and waiting for a GETDATA does mitigate this substantially: when the txs are duplicates, you're now only receiving the w/txid instead of the full transaction, reducing the wasted bandwidth by perhaps a factor of at least ~4x (32B vs ~128B for parent txid/sig/output)
...

💬 ajtowns commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#discussion_r1717682181)
`ReceivedResponse` already does this, could just have it return true in that case, and false otherwise (instead of void), and issue the error if neither the txid or wtxid resulted in a true result?

💬 agroce commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2290178998)
@maflcko @dergoegge this is actually a question worth trying muttfuzz on: I think clearly full suite / max len fuzzing kills more mutants. but I can see how big the cost is for max len 50 runs, and how common mutants that are much easier to detect with short inputs only are. that might be worth seeing, since the gap in detection time here does seem to be huge (I have many finds with 50 length corpus/max_len, some in less than 10 minutes, and the original run with full corpus/no limit is still
...

🤔 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