✅ willcl-ark closed a pull request: "build: debug enable addrman consistency checks"
(https://github.com/bitcoin/bitcoin/pull/27300)

💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1243753860)
Done.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1243773140)
Yeah my thought was that this notification wasn't something that made sense for the background chain -- the existing UI notifications for should be oblivious to background validation (and if we want to change that then we likely need new functionality/a new interface for whatever notifications we want to set up).

💬 sipa commented on pull request "test: add python implementation of Elligator swift":
(https://github.com/bitcoin/bitcoin/pull/24005#discussion_r1243781439)
Given that it's reduced to an `FE` anyway, arguably the correct value is `FE(random.randrange(1, FE.SIZE))`.

To be clear, there is no observable difference between using `GE.ORDER`, `2**256`, or `FE.SIZE`; they're all sufficiently close together that it doesn't matter, and the ElligatorSwift code in libsecp256k1 also relies on the fact that they're so close together.

But the "we prefer uniform u32 over uniform u" doesn't really apply here, as u is a field element, not a 32-byte array. In o
...

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

```
Backtrace:
In srfi/srfi-1.scm:
586:29 19 (map1 (#<<manifest-entry> name: "python-lief" versio?> ?))
586:17 18 (map1 (#<<manifest-entry> name: "powerpc64-linux-gnu-t?>))
In guix/profiles.scm:
1930:19 17 (_ _)
In guix/packages.scm:
1348:17 16 (supported-package? #<package powerpc64-linux-gnu-tool?> ?)
In guix/memoization.scm:
101:0 15 (_ #<hash-table 7f27a126c4c0 282/443> #<package powerp?> ?)
In guix/packages.scm:
131
...

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