💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903257389)
Nit: I don't think adding these comments after an include is useful. They can become stale easily and it adds to the general inconsistencies of the codebase. Can you remove it again?

💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903271412)
Is there a material difference between this test block and the previous one that actually exercises the default coinbase weight? Could more exact weight accounting where the total weight is checked with `assert_equal` instead of `assert_greater_than_or_equal` in `verify_block_template` be useful?

💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903265703)
Nit: Since you are creating a constant for the normal feerate, maybe create one for the fee rate of the large transactions too?

💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903276616)
Does this have an effect on the log line in `miner.cpp::158`? Maybe clarify there that the numbers are given without the coinbase?

💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903268301)
Nit: This is a bit confusing, I think something like `(LARGE_TXS_COUNT - 1) + NORMAL_TXS_COUNT` would be more accurate, or just give the number like you do in the next test block.

💬 TheCharlatan commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1903278355)
I'm a bit confused by this second sentence. How does the behaviour introduced here allow for overriding the default?

✅ fjahr closed a pull request: "sync: improve CCoinsViewCache ReallocateCache - 2nd try"
(https://github.com/bitcoin/bitcoin/pull/30370)

💬 fjahr commented on pull request "sync: improve CCoinsViewCache ReallocateCache - 2nd try":
(https://github.com/bitcoin/bitcoin/pull/30370#issuecomment-2571656801)
Alright, closing if the effect isn't significant. I have a few too many things on my plate at the moment anyway so maybe someone else wants to investigate further from here.

📝 kehiy opened a pull request: "doc: upgrade Bitcoin Core license to 2025"
(https://github.com/bitcoin/bitcoin/pull/31605)

💬 fjahr commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1903304101)
fixed

💬 fjahr commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1903304182)
`hasBlocks()` is already used one line above where I use `getPruneHeight()`. The problem is it doesn't tell us if we have been pruning or not.

Is there a concerns with exposing `GetPruneHeight()` explicitly? I think the function could be moved outside of RPC code, it just wasn't needed anywhere else so far.

💬 fjahr commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1903304370)
Yeah, this works, I replaced it. Thanks!

💬 fjahr commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2571684342)
Addressed feedback from @mzumsande

💬 Eunovo commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1903304573)
IIUC trying to infer a multisig redeem script that is too large under a p2sh context will return `addr(...)#...` instead of nullptr. Is this what you were going for?

💬 Eunovo commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1903306172)
> IIUC trying to infer a multisig redeem script that is too large under a p2sh context will return `addr(...)#...` instead of nullptr. Is this what you were going for?

This diff ensures that `nullptr` is returned under p2sh context (if that's what you were going for) and adds a test case that covers this if block.

```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index a2eb764706..7bc2f8cc88 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cp
...

💬 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-2571716425)
> Sample descriptors: `tr(musig([15d62cdf/87'/0'/0']xpub6CpM1svHYyNMTVdmDh5syFXCJHKctJNajbyLEdA8pAgAeg1jotmg9G1aVkND5Rzf37uhwhs8o2Lvq22iRpWwcbNGCrYxAozQfYQYi1eduES/**,[7f15646b/87'/0'/0']xpub6ChFTmSdBrhN3D16Rna7hJVQe4w56Gx83U4uNhT3oJaEXiPv7LKnY2gXi3FbbusCb145c3SMEUsSLMRdkxa82MNKqkatnK5b77BXPc3aK8h/**),{{{pk(musig([15d62cdf/87'/0'/0']xpub6CpM1svHYyNMTVdmDh5syFXCJHKctJNajbyLEdA8pAgAeg1jotmg9G1aVkND5Rzf37uhwhs8o2Lvq22iRpWwcbNGCrYxAozQfYQYi1eduES/**,[07895d1c/87'/0'/0']xpub6DF4oz8Ws6Qcd87qKeKFJCMMvc
...

🤔 theStack reviewed a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2530939442)
Concept ACK

💬 theStack commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1903322566)
in commit 850962d0074683cccbe0f63c864a70e0f31a8caf: is this a left-over from debugging or introduced intentionally?

💬 theStack commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1903312194)
seems like the picked commit 9c21b8f581542aee0f599f1d8996ac97071bdf6f is out-dated, as this sub-test was already changed to use `generate_keypair` in #30328 (see e.g. https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1878288769)

👍 TheCharlatan approved a pull request: "util: Add missing types in make_secure_unique"
(https://github.com/bitcoin/bitcoin/pull/31464#pullrequestreview-2530954372)
ACK fa397177acfa1006ea6feee0b215c53e51f284de