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

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1621275746)
Could we add `Assume()`/`Assert()`s rather than comments here and above?

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1621072544)
If this is true, this helps my understanding of the code to follow, knowing that each loop below will eventually terminate.
```Suggestion
unsigned aux = (((args >> 6) & 3) * sim.size()) >> 2;
// Args are in range for non-empty sim, or sim is completely empty and will be grown
assert((sim.empty() && dest == 0 && src == 0 && aux == 0) ||
(!sim.empty() && dest < sim.size() && src < sim.size() && aux < sim.size()));

```

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1624946699)
did you consider using `std::popcount` instead?

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1624933811)
In commit "kernel: De-globalize fReindex" (b47bd959207e82555f07e028cc2246943d32d4c3)

> I think it would be less fragile to assign `blockman_opts.reindex` instead of `chainman.m_blockman.m_reindex`.

This suggestion was not implemented in this PR, but is implemented in followup PR #30132.