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

💬 MarcoFalke commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1243941211)
You can see the early return in LoadWallet in master here:

https://github.com/bitcoin/bitcoin/blob/7ee41217b3b3fe4d8b7eb4fd1d4577b9b33d466d/src/wallet/walletdb.cpp#L863

In this pull you can see that it calls `std::max` and then continues with LoadWallet:

https://github.com/bitcoin/bitcoin/blob/3c83b1d884b419adece95b335b6e956e7459a7ef/src/wallet/walletdb.cpp#L1165-L1168

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1243950140)
What I meant was that there are records that now early return but didn't previously, but I suppose that's the opposite of what you were talking about.

💬 ryanofsky commented on pull request "util: Safer MakeByteSpan with ByteSpanCast":
(https://github.com/bitcoin/bitcoin/pull/27973#issuecomment-1609772977)
> > 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 withou
...

💬 sipa commented on pull request "util: Allow std::byte and char Span serialization":
(https://github.com/bitcoin/bitcoin/pull/27927#issuecomment-1609784685)
utACK fa38d862358b87219b12bf31236c52f28d9fc5d6

I do wonder: is there any reason why we wouldn't want to support serializing `Span`s in general (where its serialization is defined as the concatenation of the serialization of its elements)?

👍 theStack approved a pull request: "Introduce secp256k1 module with field and group classes to test framework"
(https://github.com/bitcoin/bitcoin/pull/26222#pullrequestreview-1501307590)
re-ACK d4fb58ae8ae9772d025ead184ef8f2c0ea50df3e

(as per `$ git range-diff ab30e81b...d4fb58ae`, verified that the only changes since the last ACK were a rebase on master and https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1235947681 tackled)

💬 sipa commented on pull request "util: Safer MakeByteSpan with ByteSpanCast":
(https://github.com/bitcoin/bitcoin/pull/27973#issuecomment-1609794629)
FWIW, I don't believe `AsBytes` (or `std::as_bytes`) are ever undefined behavior; every object has a memory representation, which you're allowed to observe. It is of course always implementation-defined behavior when anything but byte and byte-like arrays are involved, but the point of these functions is allowing access to that.

💬 MarcoFalke commented on pull request "util: Allow std::byte and char Span serialization":
(https://github.com/bitcoin/bitcoin/pull/27927#issuecomment-1609799341)
> I do wonder: is there any reason why we wouldn't want to support serializing Spans in general (where its serialization is defined as the concatenation of the serialization of its elements)?

I think that should be fine to add, once there is a use case. Or is there already one?

💬 kristapsk commented on issue "RFE: Add "Edit Label" for addresses in Receive tab ("Requested payments history")":
(https://github.com/bitcoin-core/gui/issues/415#issuecomment-1609799711)
Agree, this would be very useful feature. Currently there is no way to edit label for receiving addresses neither from "Receive" tab nor coin selection dialog. User could make a typo when initially creating receive address or he would like to add comma separated list of associated inputs with change when doing manual control and end up using different set of inputs as initially thought.

💬 ryanofsky commented on pull request "util: Safer MakeByteSpan with ByteSpanCast":
(https://github.com/bitcoin/bitcoin/pull/27973#issuecomment-1609823534)
> FWIW, I don't believe `AsBytes` (or `std::as_bytes`) are ever undefined behavior

Yes that should be true as long as span is pointing to live memory. I should replace "platform specific or undefined" with "implementation specific" in the #27978 change description