💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1608188265)
Rebased

👍 ryanofsky approved a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1499361716)
Code review ACK 973d910c287867d8bb1c8041bca906c14596aeda

Looks good, probably ready for merge with a third ACK.

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1242724102)
re: https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1242640941

> In [dc1ef26](https://github.com/bitcoin/bitcoin/commit/dc1ef26e430abbf92ef39d2ce595adac651b949a):
>
> nit: this result is also being set to `CORRUPT` when `LoadToWallet` returns false with `corrupted_tx=true` (~30 lines of code below this line).

Nice catch. Would be nice to just eliminate the CORRUPTED_TX variable:

```diff
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -1065,10 +1065,9 @@ st
...

📝 sipa opened a pull request: "Fix potential network stalling bug"
(https://github.com/bitcoin/bitcoin/pull/27981)
See https://github.com/ElementsProject/elements/issues/1233. There, it has been observed that if both sides of a P2P connection have a significant amount of data to send, a stall can occur, where both try to drain their own send queue before trying to receive. The same issue seems to apply to the current Bitcoin Core codebase, though I don't know whether it's a frequent issue for us.

The core issue is that whenever our optimistic send fails to fully send a message, we do subsequently not even
...

💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1608209183)
cc @psgreco

💬 achow101 commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1242737788)
To clarify, the order matters because the ECDH is more than just plain ECDH - it hashes the ECDH secret with the pubkeys, and so ordering in the hashing serialization matters. BIP 324 specifies the preimage of this hash as `initiator pub || receiver pub || ecdh secret`, so we do this swapping so that we are computing this order correctly.

💬 ismaelsadeeq commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1242750105)
Could we perhaps consider relying on the check here? It seems like a duplicate of the same process in two different places if you agree.

💬 achow101 commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#issuecomment-1608232026)
ACK 3168b08043546cd248a81563e21ff096019f1521

💬 psgreco commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1608257369)
concept ack 5e9237891df69876e1a6f81bf158aed2a683ffe2, this potential issue is not easy to trigger on demand in bitcoin, but it's relatively easy to trigger in elements, when the node is roughly 20/24 hours behind. Tested in elements a similar version of the patch it does solve the stalling

🚀 achow101 merged a pull request: "BIP324: ElligatorSwift integrations"
(https://github.com/bitcoin/bitcoin/pull/27479)

💬 tobtoht commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1608279863)
Guix builds:

```
109ed45706b8cbdc897b6c51efa0247f8e8f1e6bb35c57e69f30093b62189a51 guix-build-3df60704661c/output/aarch64-linux-gnu/SHA256SUMS.part
4e28463f1c074c7c121abd5fbad89aa2269c5978829ccd4898d33a21eedfeb4f guix-build-3df60704661c/output/aarch64-linux-gnu/bitcoin-3df60704661c-aarch64-linux-gnu-debug.tar.gz
f20d074f0529202274a40f0530f79f601f5c575f1cd0addc099418719982485e guix-build-3df60704661c/output/aarch64-linux-gnu/bitcoin-3df60704661c-aarch64-linux-gnu.tar.gz
414d7c4bf531a8d85
...

💬 achow101 commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1242859628)
Done

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

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

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1242865250)
Removed `corrupted_tx` as suggested.

💬 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-1608385210)
```
2023-06-26T19:44:34.554349Z [net] received: tx (313 bytes) peer=44
2023-06-26T19:44:34.554644Z [mempool] AcceptToMemoryPool: peer=44: accepted 783c947208643cb757b916a9693fcbbac89ae4059ec748ff8c78735739c831bc (poolsz 21998 txn, 70199 kB)
2023-06-26T19:44:34.554678Z [net] received: tx (313 bytes) peer=44
2023-06-26T19:44:34.554966Z [mempool] AcceptToMemoryPool: peer=44: accepted ccec8d2d77fcfca2386280efaff17fa5b3011d8bae3b91a2fdd2345a1ee9e5b8 (poolsz 21999 txn, 70200 kB)
2023-06-26T19:44:
...

👍 ryanofsky approved a pull request: "index: improve initialization and pruning violation check"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1499453218)
Code review ACK 3514f768b181d359d0218845ace0266d37e9dfbd

This looks good, and the PR seems pretty easy to review now. It's a series of simple, clean improvements that make index startup more straightforward and efficient.

I left two review comments, one is about a potential bug in an intermediate commit, but it's fixed by the final commit, and also not hard to address if you want to take the suggestion there.

I'd consider maybe renaming the PR from "index: improve initialization and pru
...

💬 ryanofsky commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1242783297)
In commit "init: don't start indexes sync thread prematurely" (d7a13cb990826d0422e2135a6fd4f2b8813b6eb8)

I don't think replacing the `&& g_indexes_ready_to_sync` condition with `&& !LoadingBlocks` here in this commit is correct or preserves behavior. The goal of having the condition is to skip checking code and avoid the prune_violation error below if `-reindex-chainstate` is used, but `!LoadingBlocks` will still be true if `-reindex-chainstate` is used. Definition of `LoadingBlocks` is `m_im
...

💬 ryanofsky commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1242830869)
In commit "refactor: simplify pruning violation check" (1f88e942d34bf8c51374567e9086f5535f0bde21)

Looking at the `CheckBlockDataAvailability` and `GetFirstStoredBlock` functions more closely, I don't think is ever a legitmate reason to call them with an upper block that missing data, or to call them with a lower block that is not an ancestor of ancestor of upper block.

I think if either of these things happen, something has gone badly wrong, and it would be better to treat these things as
...

💬 ryanofsky commented on pull request "refactor: Drop unsafe AsBytePtr function":
(https://github.com/bitcoin/bitcoin/pull/27978#discussion_r1242877722)
Nice, I like the use of first() and last() to avoid the pointer math