💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938054955)
Indeed, Fixed

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938055136)
I've removed this sentence since we allow `DEFAULT` and `ALL`.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938055244)
Done

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938055281)
Done

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938059816)
These checks all occur inside of `migrate_and_get_rpc` now so they're redundant.

💬 achow101 commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938074664)
The verb "fill" suggests to me that the caller is filling the descriptor with a private key, rather than the descriptor filling something else. I think "Get" is still appropriate.

💬 achow101 commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938075462)
Valid means that the origin is correct. It may not be useful, e.g. the origin of a random key is itself, but it's valid and correct.

💬 achow101 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1938079255)
This PR is big enough already, I'd prefer to do that separately.

💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628555787)
> No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I discovered the issue.

Maybe I am still not getting it but re-reading the release notes once more, I don't think they reflect this correctly. They seem to suggest that setting `-blockmaxweight` to `4,000,000` does change something. I think they need to explain what is actually happening so that miners can make the right decision. Worst case (though maybe a bit u
...

💬 achow101 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1938086462)
No... that's not at all how deserialization works. There's no constructors called, `prev_txid` is an already initialized `uint256`. Unserializing into it means that the data is overwritten. There's no `std::make_shared` called anywhere, nothing is constructed or initialized.

The call chain is `UnserializeFromVector` -> `UnserializeMany` -> `uint256::Unserialize()` -> `Unserialize(Stream& s, Span<B> span)` -> `<template Stream> read()`, and every `read(Span)` function checks that the stream ha
...

🤔 mzumsande reviewed a pull request: "kernel: Flush in ChainstateManager destructor"
(https://github.com/bitcoin/bitcoin/pull/31382#pullrequestreview-2587949312)
Concept ACK

💬 mzumsande commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1938025806)
Not too familiar with the shutdown sequence - I don't fully understand why we need to flush the chainstates twice.
Do you know any examples of events that result in the second flush on destruction doing any actual work in bitcoind?

📝 tiagosh opened a pull request: "rpc: collect transaction fees on generateblock"
(https://github.com/bitcoin/bitcoin/pull/31775)
Fixes #31684

Currently `generateblock` will insert the specified transactions directly into the new block without collecting fees in the coinbase transaction.

This patch changes the code to take transaction fees into account and make it behave closer to the expected miner behavior in mainnet.

💬 Tiffanywaters92 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_r1938118477)
So can I ask the worst question lol what is it that's wrong or needed with bitcoin with all of this? Simplest like for dummies kinda answer

💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2628621716)
Marking this as ready for review as it no longer has any prerequisites.

👋 achow101's pull request is ready for review: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250)

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121255)
I think it's possible, but I can't figure it out right now.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121303)
Done

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121319)
Done

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121540)
I changed it to `Assume` and added an early return here so that the failed migration is cleaned up.