💬 hebasto commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2145549461)
Can be closed as fixed by https://github.com/bitcoin/bitcoin/pull/30122.

✅ fanquake closed an issue: "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows"
(https://github.com/bitcoin/bitcoin/issues/29816)

💬 achow101 commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2145621189)
> @achow101 I think this is what you might not be clear about. `BatchWrite` iterates the entire cache, but this patch does not. After this patch `BatchWrite` only iterates flagged entries, which is a much smaller subset of the cache.

Yes, that was what I was missing. It just wasn't clear to me from the description that this was implemented as another performance improvement in addition to clearing dirty and spent UTXOs.

👍 ryanofsky approved a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2094278459)
Code review ACK 468cf536eb6473b0fbc1d0f9763c8d6caedeb849. This PR is a nice change making behavior more consistent, cleaning up the kernel interface, and removing globals. Main changes since last review are switching from string to enum ids and documenting behavior changes.

💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1624697758)
In commit "introduce and use the generalized `node::Warnings` interface" (cb9a8f479b278b83916841db8395c93b10107d6b)

Instead of rewriting this function and in this commit and then inlining and deleting it in the last commit, would be nice to just inline and delete it here.

💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1624714411)
re: https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612228366

> It seems undesirable that we would ever want to create a warning, and then not show it in the GUI until another - unrelated - warning is created that does trigger the GUI update. So, if we agree that we always want to update the GUI when the warnings are modified, then I think just doing it inside the `Set()` and `Unset()` methods is the most sensible approach?

Good point, I didn't realize that was what was happenin
...

📝 davidgumberg opened a pull request: "Lint: Support running individual lint checks"
(https://github.com/bitcoin/bitcoin/pull/30219)
This PR was split out from #29965:

Adds support for running individual tests in the rust lint suite by passing `--lint=LINT_TO_RUN` to the lint runner. This PR also adds a corresponding help message.

When running with `cargo run`, arguments after a double dash (`--`) are passed to the binary instead of the cargo command. For example, in order to run the linter check that tabs are not used as whitespace:

```console
cd test/lint/test_runner && cargo run -- --lint=tabs_whitespace
```

💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624501620)
In dca1ca1f56d8e63929eb7aea64f8fb3e01752357: I think `BlockManager::GetFirstBlock()` can be made `const`, which allows us to pass a `const BlockManager& blockman` here:

<details>
<summary>git diff on dca1ca1f56</summary>

```diff
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index 18eb2dc1aa..6dbc111eb6 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -595,7 +595,7 @@ bool BlockManager::IsBlockPruned(const CBlockIndex& block)
return
...

💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624736686)
nit: `peer_id` makes things less readable imo
```suggestion
node.getblockfrompeer(fetch_block, peers[0]["id"])
```

💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624683314)
I think this is buggy, chain tip should be considered pruned if any of the `BLOCK_HAVE_MASK` flags are missing:

```suggestion
if (!((chain_tip->nStatus & BLOCK_HAVE_MASK) == BLOCK_HAVE_MASK)) return chain_tip->nHeight;
```

Can be verified by adding unit test to the first commit, which passes on dca1ca1f56 and fails on current HEAD for `CheckGetPruneHeight(blockman, chain, 100)`:

```
$ ./src/test/test_bitcoin --run_test=blockchain_tests/get_prune_height
...
Running 1 test case..
...

💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624721555)
I don't think this is fully addressed in 0501ec102f3740049bd2e891cd83527c31b29222 - would update to `return Assert(first_unpruned.pprev)->nHeight;`?

⚠️ ffrediani opened an issue: "Improve/simplify node sync for pruned nodes"
(https://github.com/bitcoin/bitcoin/issues/30220)
### Please describe the feature you'd like to see added.

When running a pruned node it means it will keep only last X amount of GB predefined in the configuration, so why is it necessary to download the whole blockchain if only a tiny part will be kept at the end ? Can't it just download the amount of necessary and most recent data or a method developed for it and to speed up adoption of pruned nodes that will never keep all that amount of data ?

### Is your feature related to a problem, if so
...

💬 davidgumberg commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2145640002)
> Any thoughts on splitting the first commit out into a separate pull? Seems to be a requested feature by three people so far, but I am not sure about bundling it for review with the other changes here.

You are right, split out the first commit into: #30219

💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1624753063)
Thanks, I'll address this if I have to force push!

👍 ryanofsky approved a pull request: "prune, rpc: Check undo data when finding pruneheight"
(https://github.com/bitcoin/bitcoin/pull/29668#pullrequestreview-2094387578)
Code review 0501ec102f3740049bd2e891cd83527c31b29222. Code looks very good, but the `nStatus & BLOCK_HAVE_MASK` bug found by stickies-v https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624683314 needs to be fixed, and the other suggested changes could be considered too

💬 ryanofsky commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624762665)
In commit "refactor, blockstorage: Generalize GetFirstStoredBlock" (dca1ca1f56d8e63929eb7aea64f8fb3e01752357)

I think the code would be simpler (and have fewer cases to think) about if this function just returned -1 when no blocks were pruned, instead of nullopt:

```diff
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -782,8 +782,8 @@ static RPCHelpMan getblock()
};
}

-//! Return height of highest block that has been pruned, or std::nullopt if no blocks have bee
...

💬 ryanofsky commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624770978)
re: https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624683314

> I think this is buggy, chain tip should be considered pruned if any of the `BLOCK_HAVE_MASK` flags are missing:

Nice catch! Would be great to add the test to the first commit.

💬 m3dwards commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#issuecomment-2145726315)
@maflcko happy to keep the job together as it is. I will look into the functional tests that require IPV6.

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1624812617)
Thanks @furszy

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1620960269)
can you motivate this seed in a comment?