💬 ismaelsadeeq commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1624500904)
Should we replace all v3 mentions with TRUC for clarity and uniformity.
The BIP specification refers to this policy rules as TRUC Transactions.

💬 hebasto commented on issue "Guix builds are affected by external environment":
(https://github.com/bitcoin/bitcoin/issues/29754#issuecomment-2145371780)
> Do the shasums for the 22 LTS result differ from the 24 LTS result?

Ubuntu 24.04, x86_64:
```
7ee2be332a9c7221280fa37c5720ee93077d6aa3f1730b75d748b3fa74efea5c guix-build-80bdd4b6beb8/output/arm64-apple-darwin/SHA256SUMS.part
bac5e4d4e4f823ce020ff82caf9fed0d00006204c3fd1207db3f6642db25306a guix-build-80bdd4b6beb8/output/arm64-apple-darwin/bitcoin-80bdd4b6beb8-arm64-apple-darwin-unsigned.tar.gz
eba8ad10522dd1420436c4f1560bb8cc4c00a40db5af5cc1da782f8636603719 guix-build-80bdd4b6beb8/out
...

💬 sr-gi commented on issue "Erlay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/28646#issuecomment-2145371987)
> I've updated the PR to review, at the top of the issue here. @sr-gi are you going to open your own tracking issue going forward? That'll likely be easier to keep up to date.

Sure, if there is review traction, I'll open a new tracking issue once the current target PR gets merged

💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1624612271)
Loading the entire wallet just to obtain the descriptors flag shouldn't be needed. Could just check the file type instead. We want a bdb file here. So, something like https://github.com/furszy/bitcoin-core/commit/7a03a039ae99e69504fe9df95b04f5e326c45a70 would work well. Yet, the code isn't the best but it includes a test that verifies the behavior.

📝 theStack opened a pull request: "refactor: remove unused `CKey::Negate` method"
(https://github.com/bitcoin/bitcoin/pull/30218)
This method was introduced as a pre-requirement for the v2 transport protocol back then (see PR #14047, commit 463921bb), when it was still BIP151. With the replacement BIP324, this is not needed anymore, and it's also unlikely that any other proposal we'd ever need to negate private keys at this abstraction level. I'd argue that this operation is usually something that should happen within a secp256k1 module (like e.g. done in MuSig2, Silent Payments...).

(If there is really demand in the fu
...

👍 theStack approved a pull request: "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN"
(https://github.com/bitcoin/bitcoin/pull/30212#pullrequestreview-2094224853)
Concept and code-review ACK aa422bcb898b7e3aa8dca912d92016bd22cf243d

I agree that the proposed names are more to the point and less confusing.

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