💬 furszy commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1242640941)
In dc1ef26e:

nit: this result also is being set to `CORRUPT` when `LoadToWallet` returns false with `corrupted_tx=true` (~30 lines of code below this line).

💬 furszy commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1242385895)
```suggestion
err = "Error reading wallet database: Default Key corrupt";
return DBErrors::CORRUPT;
```

💬 dscotese commented on issue "depends build fails macOS intel":
(https://github.com/bitcoin/bitcoin/issues/27977#issuecomment-1608116683)
Maybe I'm misreading your code, but the first line ends with a dot. That tells me that you're asking the OS to make a directory named '.'. But the dot is reserved to mean the current directory. If you remove the dot and it works, please close this issue. On the other hand, if there's something I should learn about the mkdir command on a Mac, please let me know.

👍 Dezzj approved a pull request: "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode"
(https://github.com/bitcoin/bitcoin/pull/27050#pullrequestreview-1499284492)
Approved

👍 Dezzj approved a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-1499285596)
Approved

⚠️ brunoerg opened an issue: "fuzz: connman, `m_nodes` is always empty"
(https://github.com/bitcoin/bitcoin/issues/27980)
`m_nodes` in `CConnman` is always empty in connman target (since we don't call `CreateNodeFromAcceptedSocket`/`OpenNetworkConnection` for an obvious reason). Because of this, the calls for `DisconnectNode`, `FindNode`, `ForEachNode`, `GetNodeStats` and many other ones seems to be "useless". That's the reason we're not getting coverage (at all) for many of these covered functions (See: https://marcofalke.github.io/b-c-cov/fuzz.coverage/src/net.cpp.gcov.html).

I suppose we could use `ConnmanTes
...

💬 sipa commented on pull request "refactor: Drop unsafe AsBytePtr function":
(https://github.com/bitcoin/bitcoin/pull/27978#discussion_r1242690354)
This can be written to make use of `AsBytes`, actually:

```diff
@@ -472,10 +472,10 @@ struct CustomUintFormatter
if (v < 0 || v > MAX) throw std::ios_base::failure("CustomUintFormatter value out of range");
if (BigEndian) {
uint64_t raw = htobe64(v);
- s.write({reinterpret_cast<const std::byte*>(&raw) + 8 - Bytes, Bytes});
+ s.write(AsBytes(Span{&raw, 1}).last(Bytes));
} else {
uint64_t raw = htole64(v);
-
...

💬 Crypt-iQ commented on issue "depends build fails macOS intel":
(https://github.com/bitcoin/bitcoin/issues/27977#issuecomment-1608144283)
Good point - the log snippet above is from an unmodified bitcoind. But even with fixing the dot, the make target can't find `setup.py`

💬 TheCharlatan commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1608156434)
I can confirm that my guix dmg runs on macOS 11.1.

👍 TheCharlatan approved a pull request: "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM"
(https://github.com/bitcoin/bitcoin/pull/27676#pullrequestreview-1499321116)
3df60704661cdb5e61ea2b999f468f3a1d16105f

🤔 mzumsande reviewed a pull request: "fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()`"
(https://github.com/bitcoin/bitcoin/pull/27549#pullrequestreview-1499329509)
Code Review ACK 35a2175ad8bec92b409ae2202c124e39b2f3f838, haven't tested this yet, but I will let the fuzzer run for a while now.

💬 mzumsande commented on pull request "fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()`":
(https://github.com/bitcoin/bitcoin/pull/27549#discussion_r1242705238)
> Next line calling AddrMan::GetAddr() function passes std::nullopt as the network argument

no, it now passes `network` as the network argument, which, depending on the fuzzing seed, could be `std::nullopt` or any network.

The goal of this PR is to give the fuzzer the possibility to reach network-specific code paths. If there was a bug within these, it wouldn't be able to find it before, because they just weren't invoked.

💬 hebasto commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1608171885)
ACK 3df60704661cdb5e61ea2b999f468f3a1d16105f

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1608188265)
Rebased

👍 ryanofsky approved a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1499361716)
Code review ACK 973d910c287867d8bb1c8041bca906c14596aeda

Looks good, probably ready for merge with a third ACK.

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1242724102)
re: https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1242640941

> In [dc1ef26](https://github.com/bitcoin/bitcoin/commit/dc1ef26e430abbf92ef39d2ce595adac651b949a):
>
> nit: this result is also being set to `CORRUPT` when `LoadToWallet` returns false with `corrupted_tx=true` (~30 lines of code below this line).

Nice catch. Would be nice to just eliminate the CORRUPTED_TX variable:

```diff
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -1065,10 +1065,9 @@ st
...

📝 sipa opened a pull request: "Fix potential network stalling bug"
(https://github.com/bitcoin/bitcoin/pull/27981)
See https://github.com/ElementsProject/elements/issues/1233. There, it has been observed that if both sides of a P2P connection have a significant amount of data to send, a stall can occur, where both try to drain their own send queue before trying to receive. The same issue seems to apply to the current Bitcoin Core codebase, though I don't know whether it's a frequent issue for us.

The core issue is that whenever our optimistic send fails to fully send a message, we do subsequently not even
...

💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1608209183)
cc @psgreco

💬 achow101 commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1242737788)
To clarify, the order matters because the ECDH is more than just plain ECDH - it hashes the ECDH secret with the pubkeys, and so ordering in the hashing serialization matters. BIP 324 specifies the preimage of this hash as `initiator pub || receiver pub || ecdh secret`, so we do this swapping so that we are computing this order correctly.

💬 ismaelsadeeq commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1242750105)
Could we perhaps consider relying on the check here? It seems like a duplicate of the same process in two different places if you agree.