💬 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

👍 dergoegge approved a pull request: "fuzz: Flatten all FUZZ_TARGET macros into one"
(https://github.com/bitcoin/bitcoin/pull/28065#pullrequestreview-1528293665)
Code review ACK fa6fa1f7b0120c13be0a515072065db82855b111

💬 dergoegge commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1262468302)
nit: I guess we could have `g_fuzz_targets` store the opts directly? Would mean less churn when adding an option.

💬 dergoegge commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1634145483)
Concept ACK

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262178924)
This check can be omitted from commit dc2361401c `net: move CreateSock() calls from ConnectNode() to netbase methods` since the code checks `if (!sock)` earlier. `CreateSock()` will never return a non-empty `unique_ptr` that contains a `Sock` object with a bogus internal file descriptor (`INVALID_SOCKED`).

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262322061)
The interface of taking `unique_ptr` as an argument and returning the same is a bit weird. Usually functions take `unique_ptr` when the ownership of the object is transferred from the caller to the function. But in this case it is transferred to the function which then returns it back to the caller. It would be better to have an interface like:

```cpp
bool ConnectToSocket(const Sock& sock, ...);

// and call it like:

sock = ... // is unique_ptr<Sock>
if (!ConnectToSocket(*sock, ...)) {
...

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262327319)
Make the string argument const reference and the function `static` since it is used only in `netbase.cpp`:

```suggestion
static std::unique_ptr<Sock> ConnectToSocket(std::unique_ptr<Sock> sock, struct sockaddr* sockaddr, socklen_t len, const std::string& dest_str, bool manual_connection)
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262368024)
There is no need to use the generic `sockaddr_storage` since we know it is going to be `sockaddr_un`. Also, clear it for compatibility, see the example in https://www.man7.org/linux/man-pages/man7/unix.7.html

```suggestion
struct sockaddr_un addrun;
memset(&addrun, 0, sizeof(addrun));
```