💬 ryanofsky commented on pull request "refactor: Drop unsafe AsBytePtr function":
(https://github.com/bitcoin/bitcoin/pull/27978#issuecomment-1608423449)
Updated 3fca8c3c7501b7a8e0a3adbbf6c78bb446e60924 -> 2109a147406247bb654ee5fb8421a8594919fbd5 ([`pr/noptr.1`](https://github.com/ryanofsky/bitcoin/commits/pr/noptr.1) -> [`pr/noptr.2`](https://github.com/ryanofsky/bitcoin/commits/pr/noptr.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/noptr.1..pr/noptr.2)) using `AsBytes` and a `SpanFromDbt` function to avoid `reinterpret_cast` in a few places

💬 dscotese commented on issue "depends build fails macOS intel":
(https://github.com/bitcoin/bitcoin/issues/27977#issuecomment-1608428937)
I don't yet understand permissions well enough on MacOS, but if you can see /Users/nsa/bitcoin/depends/setup.py and Python can't, checking for the account under which Python runs might be a good idea. Can you invoke /usr/local/Cellar/python@3.11/3.11.3/Frameworks/Python.framework/Versions/3.11/Resources/Python.app/Contents/MacOS/Python from the same place you invoke make?

💬 MarcoFalke commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1608779293)
Can you try with my suggested diff?

```diff
diff --git a/src/net.cpp b/src/net.cpp
index a96ffcfbe9..6c9f928f0d 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -854,6 +854,10 @@ size_t CConnman::SocketSendData(CNode& node) const
node.nSendBytes += nBytes;
node.nSendOffset += nBytes;
nSentSize += nBytes;
+ if (node.nSendOffset > data.size()) {
+ LogPrint(BCLog::NET, "socket sent %s bytes (total: %s, offset: %s) for peer=%d.\n
...

💬 MarcoFalke commented on pull request "refactor: Drop unsafe AsBytePtr function":
(https://github.com/bitcoin/bitcoin/pull/27978#issuecomment-1608855369)
Needs rebase

lgtm ACK 2109a147406247bb654ee5fb8421a8594919fbd5 🐭

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

⚠️ n1kcha opened an issue: "Wallet not loaded"
(https://github.com/bitcoin/bitcoin/issues/27982)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Suddenly wallet not loaded. I was resyncing blockchain (now synced). The wallet is old and has balance. Yesterday there was no problem.
Recently updated for V24 to V25. All was working ok until today. Node is not pruned.

Disk that blocks are stored is external Toshiba with USB connection, i have just a soft link between .bitcoin/blocks to blocks at Toshiba. I have repaired this disk a
...

🤔 S3RK reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1466867248)
Did a first pass of code review. Overall code looks really good, need to spend more time reviewing the tests though.

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1241791109)
in "Amend bumpfee for inputs with overlapping ancestry" 56139fd808250fcb693b5f185e7e510804a33470

nit: you can drop this method if you decide to have `GetTotalBumpFee()` instead

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1221051466)
in "Amend bumpfee for inputs with overlapping ancestry" 56139fd808250fcb693b5f185e7e510804a33470

nit: while you're here, should we drop earlier calls to `ComputeAndSetWaste` and just call it once for all eligible results before selecting the best one?

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1238118450)
in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893

nit: suggestion
```cpp
for (auto& output : result.coins.All()) {
output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
}
```

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1243220924)
in "Amend bumpfee for inputs with overlapping ancestry" 56139fd808250fcb693b5f185e7e510804a33470

nit: you can use `SelectionResult::GetTotalBumpFee()` if you decide to introduce it

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1243238714)
in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893

nit: we can encapsulate bump fee calculation within `CoinSelectionResult::GetTotalBumpFee()`. It will come handy in the next commit as well

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1243210309)
in "Amend bumpfee for inputs with overlapping ancestry" 56139fd808250fcb693b5f185e7e510804a33470

nit: we don't really need the whole wallet there, better to pass just chain interface

💬 gruve-p 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-1608956862)
ACK https://github.com/bitcoin/bitcoin/pull/27676/commits/3df60704661cdb5e61ea2b999f468f3a1d16105f

💬 MarcoFalke commented on pull request "util: Safer MakeByteSpan with ByteSpanCast":
(https://github.com/bitcoin/bitcoin/pull/27973#issuecomment-1608957974)
Maybe MakeByteSpan should just be removed?

* Currently it (as well as `Span`) accepts a range that is not borrowed (see https://en.cppreference.com/w/cpp/ranges/borrowed_range), leading to potential lifetime issues. Someone should check if making the `Span` deduction guidelines safer will automatically make the `MakeByteSpan` function safer.
* Given that `std::as_bytes` is in the standard library and we can't prevent devs from using it (or `AsBytes`) at compile time, it seems pointless tryin
...

💬 Sjors commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1608964595)
@FelixWeis do you have inbound peers? After you've tried with the extra logging, maybe also try with `-listen=0`.

I'm running master @ 679f825ba3aae45af4476f357a67f8e6ec9483e2 now with this patch on an Intel macOS 13.4.1. Will update here if something happens. I haven't noticed such crashes with v25.0 which I've run for multiple days.

📝 MarcoFalke converted_to_draft a pull request: "util: Safer MakeByteSpan with ByteSpanCast"
(https://github.com/bitcoin/bitcoin/pull/27973)
The `AsBytes` helpers (or `std::as_bytes` helpers) are architecture
dependent. For example, the below test case diff [0], applied before
this commit will pass on x86_64, but fail on s390x [1].

Fix this by replacing `AsBytes` with a new `ByteSpanCast` in the `MakeByteSpan` helper.
This will turn the test case diff into a compile failure instead of a runtime error.

[0] test case diff:

```diff
diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp
index 09f77d2b61..4
...

💬 Sjors commented on pull request "test: skip all backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#issuecomment-1608981254)
Running the old binaries without valgrind makes sense. I'll see if I can figure out how.

💬 carnhofdaki commented on issue "Wallet not loaded":
(https://github.com/bitcoin/bitcoin/issues/27982#issuecomment-1609010564)
I think this behavior is expected. Have a look at f0758d8a66 and maybe some older related changes when the default wallet creation and loading is disabled.

It makes sense for nodes that run the released binary but do not operate their own wallet (or do it differently than using the Bitcoin Core's wallet - straight via RPC calls, for example `scantxoutset` to get the list of UTXOs).

👋 MarcoFalke's pull request is ready for review: "util: Allow std::byte and char Span serialization"
(https://github.com/bitcoin/bitcoin/pull/27927)

💬 n1kcha commented on issue "Wallet not loaded":
(https://github.com/bitcoin/bitcoin/issues/27982#issuecomment-1609025219)
I have another full node v25 (similarly recently updated from v24) to a different computer. No problems there. But how to see my wallet again, downgrade to v24?

💬 MarcoFalke commented on issue "Enable consistency checks by default with `--enable-debug`":
(https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1609028182)
Closing per https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1556705320