💬 fanquake commented on pull request "scripted-diff: Remove unused leading newline in RPC docs":
(https://github.com/bitcoin/bitcoin/pull/32514#issuecomment-2884064294)
```bash
Run rpc with args ['/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/bin/fuzz', PosixPath('/

Assertion failed: (error_msg.find("trigger_internal_bug") != std::string::npos), function rpc_fuzz_target, file rpc.cpp, line 389.
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/rpc/9cfc6f75197c087f55eb7ea406ef812047989f2b"

⚠️ Failure generated from target with exit code 1: ['/Users/runner/work/bitcoin/bitcoin/ci
...

💬 fanquake commented on pull request "depends: bump to latest config.guess and config.sub":
(https://github.com/bitcoin/bitcoin/pull/32505#issuecomment-2884067533)
Guix Build
```bash
c44d180153f6844a636a0693f588fa20c45c33efd31194a9f4d02858f5eaaaf7 guix-build-486bc9179072/output/aarch64-linux-gnu/SHA256SUMS.part
1beb0041c2d22378094d38e53878a2806d5d76557ef4f9dd2f40287e66388647 guix-build-486bc9179072/output/aarch64-linux-gnu/bitcoin-486bc9179072-aarch64-linux-gnu-debug.tar.gz
d824bbf3f03bf7679f0721173dbe2387b9168ea0f0cc24f9f676af85e5527c9b guix-build-486bc9179072/output/aarch64-linux-gnu/bitcoin-486bc9179072-aarch64-linux-gnu.tar.gz
b0ddcb7739e44760b
...

💬 maflcko commented on pull request "ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now":
(https://github.com/bitcoin/bitcoin/pull/32507#issuecomment-2884068477)
> What if instead of excluding the entire test file, we only exclude the specific failing test case?

Your diff would exclude the test case for all CI runs (even CI runs not using valgrind). I think longer term it would be better to just fix it.

🤔 stickies-v reviewed a pull request: "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`"
(https://github.com/bitcoin/bitcoin/pull/32296#pullrequestreview-2844053598)
Concept ACK for reducing suppressions.

💬 stickies-v commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2091385844)
Doesn't look like a regression in this PR because the conversion was happening implicitly anyway, but, isn't this a rather dangerous thing to do? How do we know `a` is never going to be negative?

👍 hebasto approved a pull request: "ci: remove 3rd party js from windows dll gha job"
(https://github.com/bitcoin/bitcoin/pull/32513#pullrequestreview-2844036012)
ACK 2549a1075521202ee0d9fe069ba1062563ee6387, tested locally on Windows 11. Additionally tested in my personal repo with invalidated vcpkg binary cache.

💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091377191)
Why use an exception? Wouldn’t exiting with an error code suffice?

💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091379846)
GHA [docs](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#example-of-adding-a-system-path) do not suggest `-Encoding utf8`. Is it necessary?

💬 maflcko commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#issuecomment-2884160071)
lgtm ACK e62423d6f1514b022155edb5bc930cecc4236731 🛄

<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: lgtm ACK e62423d6f1514b02
...

💬 theuni commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2884164753)
Concept ACK. Neat :)

> Maybe this is not an issue for the PR, but it would be good to make clear what types of corruption BlockTreeStore can and can't detect and what types of corruption it can recover from. If it can do simple things to detect corruption like adding checksums, or to prevent it like writing to temporary files and renaming them in place, those could be good to consider.

Yeah, I think this is the heart of it. I'm onboard for a new impl outside of leveldb, but before getting
...

💬 furszy commented on pull request "ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now":
(https://github.com/bitcoin/bitcoin/pull/32507#issuecomment-2884172976)
> > What if instead of excluding the entire test file, we only exclude the specific failing test case?
>
> Your diff would exclude the test case for all CI runs (even CI runs not using valgrind).

That would be easy to fix by just providing another environment variable for Valgrind runs. But np, it was just an idea to keep the test running while the issue is being investigated.

> I think longer term it would be better to just fix it.

Hard to disagree.

💬 maflcko commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2091413180)
> Doesn't look like a regression in this PR because the conversion was happening implicitly anyway, but, isn't this a rather dangerous thing to do? How do we know `a` is never going to be negative?

It *will* be negative and is known to be, otherwise there would be no suppression needed in `master` right now.

Also, it is perfectly fine and intended to serialize any `int8_t` value (even a negative one) as `uint8_t`, because the two int types are the same width. The only requirement is that t
...

🤔 rkrux reviewed a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2843270318)
ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e

I like how this PR brings the best/last block processed updates both in memory & disk in unison, making it easier to reason about the wallet code ultimately. The detailed commit messages aids review.

I have verified that the location of such changes makes it possible for the following wallet RPCs to correctly handle best block related memory & disk updates:
```
createwallet
loadwallet
restorewallet
unloadwallet
backupwallet
```

I feel
...

💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090917963)
Within `AttachChain` itself, there is another opportunity to use `setLastBlockProcessedInMem` here:
https://github.com/bitcoin/bitcoin/blob/c779ee3a4044df3a263394bbb8b104aeeaa7c727/src/wallet/wallet.cpp#L3137-L3143

The `cs_wallet` is held at the start of this function.

💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091164443)
Though these `m_attaching_chain` updates were done in `AttachChain` function, it looks quite cleaner now to get rid of this low level boolean from wallet instance, makes it easier to wrap head around `AttachChain`.

💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091225516)
`as it may be different in case the case of a reorg.`

Redundancy if retouched.

💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090936254)
Thanks, this makes sense to me.

Limiting the writing of best block to disk mainly to cases[^1] that are actively involved in processing the new blocks seems logical. Since these cases will be updating the best block periodically, it's prudent to not interfere by doing the same via other user driven RPCs that work on different criterion as highlighted in the above comments.

> This is because they are expected to be used during normal operation when the wallet will still be handling incoming
...

💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091415690)
```diff
- assert_equal(wallet.getwalletinfo()['immature_balance'], 0) # FIXME: #31824.
+ assert_greater_than(wallet.getwalletinfo()['immature_balance'], 0)
```
Nit: The correct balance is there, can be asserted.

💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091417700)
> self.log.info("Test that wallet doesn't crash due to a duplicate block disconnection event after an unclean shutdown")

IMO the test log should be updated to mention this improvement also.
That the wallet transactions are not missed anymore in case of unclean shutdown.

💬 rkrux commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090981396)
I verified that `ScanForWalletTransactions` does update the best block locator on disk intermittently:
https://github.com/bitcoin/bitcoin/blob/c779ee3a4044df3a263394bbb8b104aeeaa7c727/src/wallet/wallet.cpp#L1875-L1883

But I don't see it updating `m_last_block_processed`, instead it reads it. Even if `m_last_block_processed` comes out to be same as the best block locator at the end in the `AttachChain` case, I find updating `m_last_block_processed` and best block locator here after rescan bet
...