💬 maflcko commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#issuecomment-2911226436)
lgtm ACK 9ee8b7f278627b917f0784adf23cbc76cae5fa0

✅ achow101 closed an issue: "bitcoin core初次打开无法生成新的收款地址"
(https://github.com/bitcoin/bitcoin/issues/32622)

💬 maflcko commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2106681301)
nit: can remove the casts

💬 maflcko commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2108280953)
I wonder if we really want to commit to the storage serialization on the rest interface. Currently in this pull, it will be re-serialized, so there is no dependency. However, with this suggestion, and a future change in the storage serialization, there will be a breaking change on the rest interface, or alternatively a re-serialization again (back to what this pull is doing).

No strong opinion, just mentioning it here.

Maybe a benchmark could help to decide if it is worth it?

💬 maflcko commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2108287018)
> GHA

I don't know about GHA, but the Cirrus tasks require manual setup, see https://github.com/bitcoin/bitcoin/blob/638a4c0bd8b53766faeb437244b2aae4eed28dcf/.cirrus.yml#L8.

(Closing conversation for now)

🤔 furszy reviewed a pull request: "wallet, rpc, gui: List legacy wallets with a message about migration"
(https://github.com/bitcoin/bitcoin/pull/32619#pullrequestreview-2869966132)
Code review ACK f3a444c45fb4bf4e51d53ebf1cf4c2631ded481c

📝 maflcko opened a pull request: "test: Add missing ipc subtree to lint"
(https://github.com/bitcoin/bitcoin/pull/32623)
The subtree docs and lint checks list the subtrees in three places, making it hard to follow and update and easy to miss one.

Fix all issues by including the missing one and removing the list in one place.

💬 willcl-ark commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2911607200)
Thanks for the feedback @purpleKarrot.

I like the sound of your approach here myself and will be happy to adapt to it. I think i'll start off by targetting 3.:

> A depends provider: This will ignore system packages and always use vendored depends. This approach will be used for reproducible builds.

...without the optionl "auto-build depends" functionality. This is close to what I have here already (and solves my immediate personal itch regarding building using depends on NixOS).

I wo
...

💬 maflcko commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2108569954)
> Since this RPC allowed all other types to be aliases for watchonly stuff

I don't think it did. It should have only allowed bool:

```cpp
if (options.type() == UniValue::VBOOL) {
// backward compatibility bool only fallback
```



> this way it doesn't throw for any callers written in that way.

You removed it from the options object as well, which has strict=true checking enabled. So I think it would be more consistent to remove it here as well, or keep it for both
...

🤔 polespinasa reviewed a pull request: "rpc: Round verificationprogress to 1 for a recent tip"
(https://github.com/bitcoin/bitcoin/pull/32528#pullrequestreview-2870288028)
ACK

The test added faf6304bdfdf228354b4072b72f4c0ef90fdaade lgtm and makes much sense.

(non-blocking) My only point is that a warning message at least would be good for regtest as being at the tip and having the verification progress < 1 when no new blocks are getting mined (in regtest) may confuse.

<details>
<summary>see proposal
</summary>

```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 880e35ee7f..b694171583 100644
--- a/src/rpc/blockchain.cpp
+++
...

👋 fanquake's pull request is ready for review: "guix: accomodate migration to codeberg"
(https://github.com/bitcoin/bitcoin/pull/32439)

🤔 Kixunil reviewed a pull request: "contrib: add xor-blocks tool to obfuscate blocks directory"
(https://github.com/bitcoin/bitcoin/pull/32451#pullrequestreview-2870286845)
Sadly, this new implementation is quite broken in multiple ways.

💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108652853)
I see you're using this to display the messages from `main`. However that will lead to horrible messages containing `Error { msg: "..." }`. I suggest you implement `Debug` manually instead just displaying the string.

💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108657220)
This loses error sources. The proper way to do this is to create a mutable string and then append the sources in a loop with `": "` as a separator. (You could also do cargo-style `"\n\tcaused by: "`).

💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108685983)
This kind of c-style handling is not idiomatic Rust. I'd do something like:

```rust
let mut args = std::env::args_os();
args.next().ok_or("Missing program name")?;
let data_dir_path = if let Some(first_arg) = args.next() {
let data_dir = if first_arg == "-datadir" || first_arg == "--datadir" {
args.next().ok_or("the datadir argument is missing a value")?
} else if let Some(data_dir) = first_arg.strip_prefix("-datadir=") {
data_dir.to_owned()
} else if let S
...

💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108718168)
A comment explaining that this doesn't need to be cryptographic would be nice.

💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108691581)
Reminds me that non-mainnet use paths like `/regtest/blocks`, perhaps we should accept `-chain` argument as well.

💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108737971)
This seems over-complicated and not needed. You're not saing anything by "avoiding the conversion" - the optimizer would take care of that for you.

💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108663194)
Using `args` is problematic for paths because it breaks for some values. `args_os` is the appropriate solution.

💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2108721393)
To be atomic, this should create a temporary file first, write it, sync it, and then rename.