💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2877201167)
> 3. The translations for the following languages, which appear to be the result of a mistake or an act of vandalism, have been discarded:
>
> * Czech (cs)
> * Danish (da)
> * Dutch (nl)

I've restored most of damaged strings for these languages on Transifex.

👍 ryanofsky approved a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2837244742)
Code review ACK e2e9c30a4d04bd8173c3386c5b97dbac227e810e just moving WriteBestBlock call from destructor back to RemoveWallet() since last review.

I left a bunch of suggestions below and repeated some of Martin's but they are all nitpicking at this point so feel free to ignore

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087136318)
In commit "wallet: Update best block record after block dis/connect" (f1f254f6c9d3cead617300367442c1bbb449af7c)

re: https://github.com/bitcoin/bitcoin/pull/30221#discussion_r1947184556

> To deal with unclean shutdowns, during disconnect the reverse order compared to blockConnected would be better: If the locator is updated first, an unclean shutdown will lead to a rescan of the disconnected block (which might do nothing but correct the locator). But if txns are set to Inactive first and ha
...

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087200068)
In commit "wallet: Replace chainStateFlushed in loading with SetLastBlockProcessed" (357a65fd426f2122f390803d572fd89d4b0177c4)

I think everything in the comment is accurate but also vague, and it raises questions about when the last scanned block would not match the last/best blocks. I think it might be better to be more concrete like:

// Set last block scanned as the last block processed, because it may be different in the case of a reorg. Also save the best block locator because rescanni
...

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087106482)
In commit "wallet: Update best block record after block dis/connect" (f1f254f6c9d3cead617300367442c1bbb449af7c)

EDIT: Martin left a similar suggestion https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2085051375. Would be nice to follow up on any of these ideas.

---

It seems like this code is redundant and could be simplified:

```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -666,12 +666,7 @@ void CWallet::SetLastBlockProcessed(int block_height, uint256 b
...

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087113393)
re: https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2085053587

In commit "wallet: Update best block record after block dis/connect" (f1f254f6c9d3cead617300367442c1bbb449af7c)

> nit: could also call just introduce and use `WriteBestBlock()` here since `SetLastBlockProcessedInMem` is already called above.

This does seem like a nice suggestion. I also wonder if we can consolidate the seperate updates in blockConnected, deleting SetLastBlockProcessedInMem at the top, and moving it
...

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087237457)
In commit "wallet: Write best block record on unload" (8ecc7653556a1977eae5a902d51a2ececde94913)

Just to be sure, current state is that if WriteBestBlock is called here, rather than being called in the destructor or being called in FlushAndDeleteWallet, the best block locator could be a little out of date and this is maybe not ideal but fine?

re: https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2864638615

> It can't be static because the `FUZZ_TARGET` below needs to access it.
...

💬 theuni commented on pull request "[28.x] build: suppress `-Wunterminated-string-initialization` in secp256k1":
(https://github.com/bitcoin/bitcoin/pull/32484#issuecomment-2877276631)
Hmm, my first thought was just to backport the "fix" instead, but I guess we've never really backported secp changes for the sake of keeping in sync.

Either way, in this case I agree it's simple/safe enough to just use the warning for 28.x.

💬 stringintech commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087260406)
As @TheCharlatan noted [here](https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2083133917), delaying `txdata.Init` could be on purpose for the potential cache hit [here](https://github.com/TheCharlatan/bitcoin/blob/16a695fbff4a92eba3eb72ec6aa766e71c52f773/src/validation.cpp#L2195:L2201). I wonder if there is a cleaner version for achieving the same goal though.

👍 hebasto approved a pull request: "[28.x] build: suppress `-Wunterminated-string-initialization` in secp256k1"
(https://github.com/bitcoin/bitcoin/pull/32484#pullrequestreview-2837501535)
ACK 5c8a55f917d089c8bdf5f59195116d9201c597e7, tested on Ubuntu 25.04 with GCC-15.

💬 fanquake commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/32479#issuecomment-2877292087)
> This is failing in the macOS CI. An issue with Boost Process:

Looks this is because Boost Process V1 compat was broken in Boost 1.88.0, https://github.com/boostorg/process/issues/480, which is the version being used in the macOS CI here, and `--enable-external-signer` is passed globally in the CI.

💬 stringintech commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2087263016)
It compiles for me as well.

👍 theStack approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2837512117)
re-ACK 35bcd8eed38130445aef5ebe217ab42248fa6f18

💬 mzumsande commented on issue "qa: Failure in wallet_basic.py spendzeroconfchange test":
(https://github.com/bitcoin/bitcoin/issues/32456#issuecomment-2877311830)
See [#32483](https://github.com/bitcoin/bitcoin/pull/32483) for a fix / way to reproduce.

💬 yancyribbens commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2087278600)
This should also return a non-zero exit code and error.

💬 brunoerg commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2087281999)
f99fd0b87dcfaf84784ce423f78a950ad377b36b: Now that you're using `ConsumeDeserializable` to create a CBlock, maybe you don't to create the transactions using `ConsumeTransaction` anymore?

💬 ismaelsadeeq commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2087286446)
> It seems like it could be more confusing to not mention one of the alternatives than just describe both.


You are right I think it's okay to include both alternative comment in the `doc/README.md`.

However In my opinion, that's sufficient, and we don't need to repeat in every instance that, for example, `bitcoin node` or `bitcoin GUI` can be substituted with `bitcoind`, etc.

📝 fanquake opened a pull request: "Update minisketch subtree"
(https://github.com/bitcoin/bitcoin/pull/32485)
Includes:
* https://github.com/bitcoin-core/minisketch/pull/89
* https://github.com/bitcoin-core/minisketch/pull/94 (in #32482)

⚠️ fanquake opened an issue: "build: deprecated arg usage in macOS deploy script"
(https://github.com/bitcoin/bitcoin/issues/32486)
`--deep` has been deprecated for at least the last 2 major versions of macos:

> --deep (DEPRECATED for signing as of macOS 13.0) When signing a bundle. ...

Given it's deprecated, I'd think that means we shouldn't actually need to use it. Assuming this also means it could be removed at some point, we should probably stop using it regardless. See our usage here:

https://github.com/bitcoin/bitcoin/blob/8309a9747a8df96517970841b3648937d05939a3/contrib/macdeploy/macdeployqtplus#L497

✅ ismaelsadeeq closed an issue: "RFC: Should node Wallet Startup Options Apply to Individual Wallets?"
(https://github.com/bitcoin/bitcoin/issues/32462)