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

💬 MarcoFalke commented on pull request "util: Safer MakeByteSpan with ByteSpanCast":
(https://github.com/bitcoin/bitcoin/pull/27973#issuecomment-1609710867)
> If I'm writing or reviewing code that uses a safe implementation of MakeByteSpan I can be more confident it is doing the right thing than if I see code using AsBytes.

Would you be less confident if there was a clang-tidy plugin doing the check? For example, instead of having two functions that do a similar thing, but it is unclear which one is safer/preferred from just looking at the name, we could have an additional `UnsafeAsBytes` (or similar), where it is clear that using it without look
...

💬 ryanofsky commented on pull request "util: Allow std::byte and char Span serialization":
(https://github.com/bitcoin/bitcoin/pull/27927#issuecomment-1609727952)
re: https://github.com/bitcoin/bitcoin/pull/27927#issuecomment-1602200826

Thanks for the responses. You convinced me there probably little benefit to restricting serialization of cha spans, and it would just add boilerplate and not make things safer.

I do think we can drop the AsBytePtr function, though, so I tried to do that in #27978.

I also still think in the longer run it would be good to convert more internal code to use `std::byte` instead of `unsigned char` and replace `UCharCast
...

💬 MarcoFalke commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609730104)
re-ACK 3c83b1d884b419adece95b335b6e956e7459a7ef 🍤

<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: re-ACK 3c83b1d884b419adece9
...