💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121727206)
re: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090831336

In commit "validation: cache all headers with enough PoW in invalidateblock" (9304257d67e6f280cc1b45021118c6bb37489d89)

> In which case, I don't think this part of the commit message is correct, at least for this commit?

Yes, I was going to ask about this same thing. I don't understand what "Since we don't remove blocks after inserting into setBlockIndexCandidates anymore" is referring to.

EDIT: I guess it refer
...

💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121807875)
re: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090961175

In commit "validation: in invalidateblock, calculate m_best_header right away" (a335d003eba7bf8204db457a2a6dcca0b2f53a43)


It does seem surprising that if both RecalculateBestHeader and the CheckBlockIndex `pindex->nChainWork <= m_best_header->nChainWork` were changed to use CBlockIndexWorkComparator the check would fail. Would be good to know what causes this and it would be good to have a comment in CheckBlockIndex
...

💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2932138830)
> This change looks ready to merge with all the reviews and discussion so far, but there are some suggestions from 3 weeks ago [#31405 (review)](https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2843060953) that could be responded to. Could merge this or hold off depending on your preference @mzumsande

Please hold off merging, I'll address @stickies-v and your comments later this week!

💬 theuni commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2932150196)
Nice! Good work, @hebasto :)

💬 achow101 commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121986744)
Added a check for `persistent`.

💬 achow101 commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121987027)
Changed.

💬 achow101 commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121987972)
It's generally a pattern in the wallet that we update in memory before writing to disk.

💬 theuni commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#discussion_r2121989228)
Do these defines actually do anything in our case, or are they just here for the sake of being explicit?

💬 achow101 commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121989490)
It is a bit weird, but changing the interface is probably not going to happen as it would be a breaking change.

💬 AMoynahan89 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2932168428)
Why is this pr still open?

"It's abundantly clear that this PR is controversial and, in its current state, has no hope of reaching a conclusion that is acceptable to everyone." -Achow101

https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1878853896

🤔 mabu44 reviewed a pull request: "policy: make pathological transactions packed with legacy sigops non-standard"
(https://github.com/bitcoin/bitcoin/pull/32521#pullrequestreview-2889699475)
ACK abd0749fae7259a292cb652fc9974c8cdcdd4d9d
Reviewed the code, compiled it, and ran the new tests. Additionally, made modifications to both the code and the tests to intentionally cause the tests to fail in expected ways.

💬 achow101 commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2122002429)
Fixed

💬 achow101 commented on pull request "wallet: Track no-longer-spendable TXOs separately":
(https://github.com/bitcoin/bitcoin/pull/27865#discussion_r2122004809)
Fixed

👍 ryanofsky approved a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-2889705787)
Code review ACK ed172d3002da68a3ddbd5d13e7d3f0618c1378d4, just rebased due to various conflicts since last review.

---

re: https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2930369097

> We should treat the `libmultiprocess` subtree the same as all other subtrees and disable the `EXPORT_COMPILE_COMMANDS` property for _all_ its targets.

This could be reasonable, though I'm not very sure what the benefits of disabling linting here would be. Also, I think even with EXPORT_COMPIL
...

💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2122005059)
In commit "build: depends makes libmultiprocess by default" (adc1bcf784ae9763246f1f143ebc225214bf1730)

Not important, but might be nice to replace `multiprocess_packages` with `ipc_packages` to be more consistent.

💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2932221955)
> @bigspider Ledger Live says my LedgerX device is up to date. The mainnet Bitcoin app version is 2.4.0, but the testnet is at 2.3.0.
>
> Or do I need the Bitcoin Test Legacy app? That one has a version 2.4.7, but it throws "ClaNotSupported" if I try it with [async-hwi](https://github.com/wizardsardine/async-hwi/tree/master/cli) `device list` command.

No, "Bitcoin Test" version 2.4.0 is the correct app; maybe something went wrong and its deployment is missing - I'll check tomorrow. What is
...

💬 achow101 commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2122024645)
Good point, dropped the apostrophe check.

💬 achow101 commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#discussion_r2122025089)
Taken suggestion, with some modifications.

💬 ryanofsky commented on pull request "wallet: Translate [default wallet] string in progress messages":
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2932279122)
Rebased a46ec1dece8d8a1b16ff1fc3d2932bca130bf67f -> db225cea56b0531cc42d4b89dc61b02890f432ff ([`pr/dtran.3`](https://github.com/ryanofsky/bitcoin/commits/pr/dtran.3) -> [`pr/dtran.4`](https://github.com/ryanofsky/bitcoin/commits/pr/dtran.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/dtran.3-rebase..pr/dtran.4)) due to conflict with #28710

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#issuecomment-2932328786)
> According to BIP 390 [_"Repeated participant public keys are not allowed."_](https://github.com/bitcoin/bips/blob/72af87fc72999e3f0a26a06e6e0a7f3134236337/bip-0390.mediawiki?plain=1#L36), but it seems there is currently no check preventing that? Example test case that passes but shouldn't (IIUC): [theStack@f260c7e](https://github.com/theStack/bitcoin/commit/f260c7ec06e8bbb7148877a9a9b7e96707c41fa1)

Good catch. Fixed and added a test.