✅ fanquake closed a pull request: "Sultan.saud@mail.ru"
(https://github.com/bitcoin/bitcoin/pull/27979)

📝 fanquake locked a pull request: "Sultan.saud@mail.ru"
(https://github.com/bitcoin/bitcoin/pull/27979)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 achow101 commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1242524517)
This function may be used by other callers (in the future).

💬 achow101 commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1242524593)
Fixed

💬 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-1607986982)
> Guix (self-signed) `bitcoin-qt` tested on macOS Ventura 13.4.1 (Apple M1).

Guix DMG tested on macOS Monterey 12.6.7 (Intel).

💬 davidgumberg commented on pull request "Remove the syscall sandbox":
(https://github.com/bitcoin/bitcoin/pull/27896#issuecomment-1607998026)
crACK https://github.com/bitcoin/bitcoin/pull/27896/commits/32e2ffc39374f61bb2435da507f285459985df9e

Nit: We may want to remove `BCLog::Util` as well, since `SetSyscallSandboxPolicy` was its only user.

🤔 furszy reviewed a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1498812845)
ACK 973d910c

Only left few more nits. Nothing blocking.

💬 furszy commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1242372035)
In 7c13da13:
Only if you have to re-touch: extra jump line

💬 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.