💬 sipa commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2297842573)
This could cause a wallet (and its backup) to have a best block locator that's unknown to the block index, if a crash happens in between the backup and the next database flush. How do we behave when loading such a wallet file?

Other than that, Concept ACK.

💬 sipa commented on pull request "http: set TCP_NODELAY when creating HTTP server":
(https://github.com/bitcoin/bitcoin/pull/30675#issuecomment-2297843609)
Concept ACK

🤔 furszy reviewed a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2246852117)
See #30221. It fixes this in a more comprehensive way.
The comment I left is secondary.

💬 furszy commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1722605697)
If the node's chain is ahead of the wallet, you would be writing a locator in the future. Which would make the wallet skip blocks during the next rescan attempt.
Should lock the wallet mutex and obtain the wallet's best locator from the wallet's last processed block (`m_last_block_processed`).

💬 achow101 commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2297854473)
> This could cause a wallet (and its backup) to have a best block locator that's unknown to the block index, if a crash happens in between the backup and the next database flush. How do we behave when loading such a wallet file?

No issues, we just rescan from the most recent fork indicated by the locator. It's the same as loading a wallet when the node isn't synced yet.

🤔 tdb3 reviewed a pull request: "http: set TCP_NODELAY when creating HTTP server"
(https://github.com/bitcoin/bitcoin/pull/30675#pullrequestreview-2246864976)
Concept ACK
Good find! Left just a nit for now, but will return to review in more detail (and possibly run some tests for different size transfers/blocks)

💬 tdb3 commented on pull request "http: set TCP_NODELAY when creating HTTP server":
(https://github.com/bitcoin/bitcoin/pull/30675#discussion_r1722614782)
nit: could use `n_one` to better align with style guidelines.

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
> Variable (including function arguments) and namespace names are all lowercase and may use _ to separate words (snake_case).

💬 sipa commented on pull request "http: set TCP_NODELAY when creating HTTP server":
(https://github.com/bitcoin/bitcoin/pull/30675#discussion_r1722617576)
Following the style guide it'd just be called `one`. The `n` prefix is a Hungarian notation prefix for "number", which we no longer use.

💬 romanz commented on pull request "http: set TCP_NODELAY when creating HTTP server":
(https://github.com/bitcoin/bitcoin/pull/30675#discussion_r1722690617)
Thanks! Fixed to `one` in c1bec82697.

💬 paplorinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1722716550)
I don't have write access to yiur PR:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews#resolving-conversations

💬 paplorinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1722725094)
If we need to document the behavior of a method, it suggests that we’ve acknowledged it may not be intuitive for everyone, indicating that we’ve conceded on finding a better design.
I think there's a way we can avoid the comment: by separating the two concerns of getting a value from cache and checking if it's spent or not.
Or if you insist, we can call it "GetUnspent" in which case we can also get rid of the comment and call-site asserts and keep the current behavior.
What do you think?

💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1722725975)
Thanks, added back

💬 maflcko commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-2298040795)
rfm with 5 acks on the first commit (2 stale and 3 current), and 3 acks on the second commit?

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2298062851)
Would be great to get the final issues resolved. Possible way forward, preserving backwards compatibility:

<details>
<summary>
diff
</summary>

```diff
diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
index ff5a7ebd00..e83b5f38b6 100644
--- a/src/node/chainstatemanager_args.cpp
+++ b/src/node/chainstatemanager_args.cpp
@@ -33,7 +33,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage

if (auto value{args.
...

⚠️ MummaStacks opened an issue: "Running Bitcoin Bitcore on new Apple M3"
(https://github.com/bitcoin/bitcoin/issues/30680)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

“Bitcoin Core” can’t be opened because Apple cannot check it for malicious software. This software needs to be updated. Contact the developer for more information."

### Expected behaviour

upload lates version of the application

### Steps to reproduce

wont allow me to install application

### Relevant log output

_No response_

### How did you obtain Bitcoin Core

Compiled from source


...

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2298084825)
Edited pull description and title to remove the mention of the global, now that it is removed in the last commit

💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-2298086275)
Is this rfm with three acks, or does it need more, or is it waiting for after the branch off?

💬 maflcko commented on pull request "MiniWallet funcitonality for more readable code in functional test `rpc_signtransactionwithkey.py`":
(https://github.com/bitcoin/bitcoin/pull/30667#issuecomment-2298095817)
You'll have to run the test locally, after you modify it and before you push the code to a pull request. There is a contributing guide for developers and how to run the tests.

👍 TheCharlatan approved a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2247125663)
Re-ACK fa899fb7aa8a14acecadd8936ad5824fa0f697ff

💬 maflcko commented on issue "Unable to build Bitcoin using './contrib/guix/guix-build'":
(https://github.com/bitcoin/bitcoin/issues/30676#issuecomment-2298116470)
There is a packaging error fixed in https://salsa.debian.org/debian/guix/-/commit/b27fcc3fa6d3633eb2beceb014507045f534f2d0

You can ask for it to be backported upstream, or work around it manually for now via `apt install netbase`