💬 ryanofsky commented on pull request "util: Safer MakeByteSpan with ByteSpanCast":
(https://github.com/bitcoin/bitcoin/pull/27973#issuecomment-1607791383)
fad126dae2bc654a28b0a77e35aa43cae42d1e0c seems like a nice improvement. It makes existing MakeByteSpan function safer by only allowing it to be called on arrays of characters, and not array of other things like `uint16_t` that could have platform specific representations. Some thoughts though:

- `MakeByteSpan` probably deserves to have some documentation since it is one of the more useful function in that file and is called a lot of places.
- I don't like introducing the `ByteSpanCast` funct
...

⚠️ Crypt-iQ opened an issue: "depends build fails macOS intel"
(https://github.com/bitcoin/bitcoin/issues/27977)
Make command:
```
make CC="/usr/local/Cellar/llvm/16.0.4/bin/clang" CXX="/usr/local/Cellar/llvm/16.0.4/bin/clang++" LDFLAGS="-L/usr/local/opt/llvm@16/lib/c++ -Wl,-rpath,/usr/local/opt/llvm@16/lib/c++" NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_UPNP=1 NO_NATPMP=1 -j 4
```

Environment: macOS intel, clang16

Last lines of output:
```
echo Building native_ds_store...
mkdir -p /Users/nsa/bitcoin/depends/work/build/x86_64-apple-darwin21.6.0/native_ds_store/1.3.0-f3b721f4d37/.
{ cd /Users/nsa/bitcoin
...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1242445552)
sorry, I guess I misinterpreted this conversation. let's clarify :)

your initial comment had two suggestions
1. remove the out param & return optional instead of bool
2. drop the optional from the out param

it looks like I focused & responded to the issue with # 1, but overlooked # 2. to restate, I think # 1 would be nice in isolation, but based on the caller, doesn't make the most sense here.

that said, # 2 of dropping the optional seems reasonable. the caller is already respons
...

📝 ryanofsky opened a pull request: "refactor: Drop unsafe AsBytePtr function"
(https://github.com/bitcoin/bitcoin/pull/27978)
Replace calls to AsBytePtr with direct calls to reinterpret_cast. AsBytePtr is just a wrapper around reinterpret_cast. It accepts any type of pointer as an argument and uses reinterpret_cast to cast the argument to a std::byte pointer.

Despite taking any type of pointer as an argument, it is not useful to call AsBytePtr on most types of pointers, because byte representations of most types will be platform specific or undefined. Because it is named similarly to the AsBytes function, AsBytePtr
...

💬 theuni 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-1607849084)
> > the runtime functionality required for `-Wl,-fixup_chains` [requires macOS >=11.0](https://github.com/llvm/llvm-project/blob/release/16.x/lld/MachO/Driver.cpp?rgh-link-date=2023-05-16T15%3A52%3A00Z#L1021).
>
> However, [Xcode 13 Release Notes](https://developer.apple.com/documentation/xcode-release-notes/xcode-13-release-notes) state:
>
> > All programs and dylibs built with a deployment target of macOS 12 or iOS 15 or later now use the chained fixups format. This uses different load c
...

🤔 D33r-Gee reviewed a pull request: "Add menu option to migrate a wallet"
(https://github.com/bitcoin-core/gui/pull/738#pullrequestreview-1499005933)
tested ACK on WSL Ubuntu 22.04
Migrated a few test wallets using the GUI on regtest, both encrypted and not-encrypted wallet yelled successful results.

📝 Sultansaudtr opened 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
...

✅ 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