🤔 furszy reviewed a pull request: "ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now"
(https://github.com/bitcoin/bitcoin/pull/32507#pullrequestreview-2843919937)
What if instead of excluding the entire test file, we only exclude the specific failing test case?

```diff
diff --git a/test/functional/wallet_reorgsrestore.py b/test/functional/wallet_reorgsrestore.py
--- a/test/functional/wallet_reorgsrestore.py (revision 8d5a11f34157934d9aecf8d5535ec3c17b13fcf3)
+++ b/test/functional/wallet_reorgsrestore.py (date 1747319065527)
@@ -14,6 +14,7 @@
"""

from decimal import Decimal
+import os
import shutil

from test_framework.test_framework i
...

🤔 mzumsande reviewed a pull request: "ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now"
(https://github.com/bitcoin/bitcoin/pull/32507#pullrequestreview-2843925049)
utACK fa981b90f53101bff2eda606d9479233e71736b5

🤔 shahsb reviewed a pull request: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-2843940627)
Concept ACK

⚠️ fanquake opened an issue: "test: failure in `mining_basic.py` AssertionError: not(4.656542373906924E-10 == 4.656542373906925E-10)"
(https://github.com/bitcoin/bitcoin/issues/32515)
x86_64 Alpine Linux
master @ c779ee3a4044df3a263394bbb8b104aeeaa7c727
```bash
/bitcoin # build/test/functional/test_runner.py mining_basic.py --valgrind --timeout-factor=0
Temporary test directory at /tmp/test_runner_₿_🏃_20250515_142647
Remaining jobs: [mining_basic.py]
1/1 - mining_basic.py failed, Duration: 58 s

stdout:
2025-05-15T14:26:48.007000Z TestFramework (INFO): PRNG seed is: 1679234749093477821
2025-05-15T14:26:4
...

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