💬 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

💬 jonatack commented on issue "Enable consistency checks by default with `--enable-debug`":
(https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1609831542)
> The drawback of setting these options on by default is that they might make a node unusable under load ([#27300 (comment)](https://github.com/bitcoin/bitcoin/pull/27300#issuecomment-1479890611) for addrman checks or #27586 for others). Due to `cs_main` and the mempool, a lot of what we do is effectively single-threaded, so using 100% of a single core can easily be a pretty severe bottleneck.

I can confirm that my node sees block download timeouts and has trouble keeping up with the mainnet
...

💬 sipa commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1609832143)
I've discussed some of the concerns here with @instagibbs, though haven't reviewed the code or read all the discussion.

As I understand it, there are (at least) these two issues to be addressed:
* If a package is received, but its overall feerate is too low, it may still be the case that there are acceptable subpackages (as the top of the graph) we would have liked to accept (and this could happen when e.g. we missed the initial announcement of those subpackages). This is a regression compar
...

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1244021660)
Agreed, done.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1244022042)
Done.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1244023980)
Done (combined with taking @ryanofsky's approach for caching this value).

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1244024243)
Agreed, gone now.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1244025126)
Can you explain a bit more what you have in mind for rewriting this function? Not sure I follow but happy to try to make this more readable/less error-prone.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1244025319)
Fixed, thanks.