💬 fanquake commented on pull request "build: LLVM 15 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1609558576)
> powerpc64-linux-gnu build is failing with:

Thanks, thats's already mentioned as a known issue here: #27897, which also does a time-machine bump. You don't need to build for powerpc to test the two macOS build, if you want to test the changes here.

💬 FelixWeis commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1609560364)
```
2023-06-27T11:10:56.851484Z [mempool] AcceptToMemoryPool: peer=8: accepted 57303c3ef5097e684419aa326d2a0048005f1571c1b26ffa315b9b8119143907 (poolsz 40693 txn, 132600 kB)
2023-06-27T11:10:57.059933Z [net] sending inv (37 bytes) peer=2
2023-06-27T11:10:57.060104Z [net] socket sent 28 bytes (total: 24, offset: 28) for peer=2.
Assertion failed: (false), function SocketSendData, file net.cpp, line 859.

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609563472)
With 3 up-to-date acks, and all the review that's happened so far, I think this looks ready to merge now. But will wait to see if @achow101 wants to respond to newest comments.

I like Marco's error handling suggestions since they simplify code and just print all the errors. If it turns out there are this cases where this repeats too many errors, it seems like it would be easy to add a bit of code to LoadRecords that just compares new error messages to previous and adds `[Repeated ## times]` t
...

💬 MarcoFalke commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609571471)
> If it turns out there are this cases where this repeats too many errors

I think that'd be the case on current master as well. My comment was that this pull may repeat the *first* error message, even though there may be a second one.

💬 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-1609577443)
So that seems to confirm my guess that this is a macOS bug? (The total data is 24 bytes, but macOS `send()` returned 28)

My suggestion would be to call Tim Cook or switch to Linux.

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609592218)
> I think that'd be the case on current master as well. My comment was that this pull may repeat the _first_ error message, even though there may be a second one.

Makes sense, I missed that. I thought the code was only trying to print one error message, and didn't notice it would print it multiple times. I like your fix of just printing all the messages instead of printing one, but either way to fix this seems fine.

💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1243851964)
Oops. Done now.

👍 ryanofsky approved a pull request: "util: Allow std::byte and char Span serialization"
(https://github.com/bitcoin/bitcoin/pull/27927#pullrequestreview-1501082371)
Code review ACK fa38d862358b87219b12bf31236c52f28d9fc5d6. This looks great. The second commit really really removes a lot of boilerplate and shows why the first commit is useful.

💬 ryanofsky commented on pull request "util: Allow std::byte and char Span serialization":
(https://github.com/bitcoin/bitcoin/pull/27927#issuecomment-1609650746)
> Fix that by introducing a helper to convert any byte-like span to a std::byte-span, which is then accepted by serialization.

Can drop this line from PR description (https://github.com/bitcoin/bitcoin/pull/27927#issue-1767744286)

💬 furszy commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609660812)
> Also, it may be good to add tests for currently untested code: https://marcofalke.github.io/b-c-cov/total.coverage/src/wallet/wallet.cpp.gcov.html#2926

I actually have a PR planned to remove most of those lines. The string errors there are duplicated from the internal walletdb errors. We can just bubble up the error instead of swallowing it.

Also, in the unit tests, we usually call `CWallet::LoadWallet` instead of `CWallet::Create`, thus why some of those paths are untested (like the `UN
...

👍 fanquake approved a pull request: "test: Add implicit-signed-integer-truncation:*/include/c++/ suppression"
(https://github.com/bitcoin/bitcoin/pull/27940#pullrequestreview-1501116675)
ACK fae55f989e2654582271af3ca635fd6c4948e3be - reproduced the failure:
```bash
Run addition_overflow with args ['/home/fedora/ci_scratch/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/home/fedora/ci_scratch/ci/scratch/qa-assets/fuzz_seed_corpus/addition_overflow')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3855426920
INFO: Loaded 1 modules (524434 inline 8-bit counters): 524434 [0xaaaabd411960, 0xaaaabd4919f2),
INF
...

🚀 fanquake merged a pull request: "test: Add implicit-signed-integer-truncation:*/include/c++/ suppression"
(https://github.com/bitcoin/bitcoin/pull/27940)

💬 achow101 commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1243894028)
I thought `1` was `OP_1`?

💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1243896823)
Not in witnesses. :)

💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1243897284)
Ah yeah.
The other possibility, which adds a bit more overhead, could be to move the pruning violation check from `Init()` to `Start()` in the intermediate commit, then move it from `Start()` to `StartIndexes()` in the last one.

Will go with your suggestion. Thanks.

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

I don't think I agree. `MakeByteSpan(x)` is just equivalent to `AsBytes(Span{x})` currently, so right now it is pointless. But if we go ahead with this PR and restrict it to only work on character arrays, that will actually make it safer and actually useful.

> it seems pointless trying to offer a safe MakeByteSpan wrapper with only a docstring suggesting to use it.

I don't think it's pointless to have a safe function even though a dangerous a
...

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1243917544)
Fixed

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1243918023)
Changed

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1243920626)
Implemented the suggested changes.

> Also, could mention that for other record types, previous early returns from LoadWallet like `UNEXPECTED_LEGACY_ENTRY` would now be scheduled for later, and it continues to load more prefix-keys?

I don't think that's necessarily true? I'm planning on revisiting the error handling in a followup.

💬 achow101 commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1243922086)
Oh right, duh.