💬 megumin9 commented on issue "The destructor of CCheckQueueControl may throw a exception ":
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1633404649)
I understand and I'm sorry to bother you.

💬 ajtowns commented on issue "Auto detect IPv6 connectivity":
(https://github.com/bitcoin/bitcoin/issues/28061#issuecomment-1633414939)
> is `EADDRNOTAVAIL` then that means the machine has no IPv6 address configured (but it may be configured later).

If the system can be reconfigured at runtime to make ipv6 available, it seems a bad idea to force the user to also have to restart bitcoind in order for bitcoind to start making/accepting ipv6 connections. (You might also have ipv6 fail due to your router/isp not supporting ipv6, which also may change at runtime)

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1261909269)
Maybe we could recover that as an invariant by having txs added in `MaybeUpdateMempoolForReorg` set the sequence number as 0? The idea being that if they've made it into a block then we've probably announced the headers already so it's no secret that we knew about the tx anyway...

💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261914712)
Done.

💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261914858)
Oh nice, I wasn't aware of `ParseHex<std::byte>`. Done.

💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261914931)
Done.

💬 MarcoFalke commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1633606181)
> Why not CXXFLAGS='$CXXFLAGS -gdwarf-4'?

Feel free to test it and make an alternative pull, but I am pretty sure it will not work. Also, I don't see any upside and it is more verbose.

💬 MarcoFalke commented on pull request "RPC: Add universal options argument to listtransactions":
(https://github.com/bitcoin/bitcoin/pull/22807#discussion_r1262056637)
```
wallet/rpc/transactions.cpp:441:75: error: no member named 'OMITTED_NAMED_ARG' in 'RPCArg::Optional'
{"options|skip", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "for backward compatibility: passing in a number instead of an object will result in {\"skip\":nnn}",
~~~~~~~~~~~~~~~~~~^
wallet/rpc/transactions.cpp:508:9: error: use of undeclared identifier 'RPCTypeCheckArgument'
RPCTypeCheckArgum
...

💬 glozow commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1262316909)
Yeah good point.

It seems that, given the need to synchronize between `TxRequestTracker` and the package tracking stuff, we should have them both guarded by 1 lock. Looking at #26151, it seems like we could have a `m_txrequest GUARDED_BY(tx_download_mutex)`, `m_txpackagetracker GUARDED_BY(tx_download_mutex)`, and lock it from these peerman functions?

Alternatively, I wonder if it makes sense to take it a step further and put this all in a `TxDownloadManager` module that wraps orphanage, tx
...

👍 MarcoFalke approved a pull request: "test: Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1528038830)
lgtm ACK fe9da891eb7a7440b6c23d63e129525fe26f8f0e 🚊

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK fe9da891eb7a7440
...

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1262326358)
d82a493ed9570e36d45ce0619691f958fad8429a: Agree with ryan that this can be run on root as well, also the function `skip_if_root` seems confusing, due to the windows handling (at least to me).

Might be better to just put a `if (root...):` guard in this line in this file only?

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1262333855)
nit in fe9da891eb7a7440b6c23d63e129525fe26f8f0e: Can just inline `echo > ~nonroot/.bitcoin` directly?

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1262301743)
2b624780b601be60700a27f893d24c0e8a11cd41: Can remove this `struct`, no? Otherwise I'd like to understand why it is needed and why it works. The `-fastprune=1` you pass isn't picked up by `blockman_opts`.

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1262331864)
Any reason to use this? I wonder if the test runs faster if you just bloat each block with 1MB of nulldata?

Something like: `d=999*"ff";generateblock(output=f"raw(6a{d})", tx=[]);`?

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1262327787)
Might be better to just assert this is `1`, or omit, as otherwise it would lead to issues anyway?

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1262338435)
nit: Shouldn't this use ceil instead of floor?

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1262340617)
(See https://github.com/llvm/llvm-project/commit/104cd749f5cca609a79303c0dad22bc041b5448a )

Switched to `auto` in any case.

💬 chinggg commented on pull request "fuzz: Modify tx_pool_standard target to test package processing":
(https://github.com/bitcoin/bitcoin/pull/25778#issuecomment-1633957629)
I am not sure if the package relay features have been implemented. Feel free to reach out to me if there is need to continue the development of this fuzz target.

💬 dergoegge commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1633985978)
Looks like this is implicitly doing the same as #28071? from the qa-assets test run: https://cirrus-ci.com/task/4623075795271680?logs=ci#L1952-L1955

```
Options used to compile and link:
external signer = no
multiprocess = no
with experimental syscall sandbox support = no
with libs = no
with wallet = yes
with sqlite = yes
with bdb = no
with gui / qt = no
with zmq = no
with test = not building test_bitcoin because fuzzing is
...

👍 dergoegge approved a pull request: "ci: Add missing -O2 to valgrind tasks"
(https://github.com/bitcoin/bitcoin/pull/28071#pullrequestreview-1528269852)
utACK fa4ccf15118a5e2dcd2a98283b9e6c7e1955a7f1