💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1161062927)
I agree, the naming here was a bit confusing. It should be fixed now.

💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1161062954)
Done both here and one other place.

💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1161062971)
Fixed

💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1500792114)
Thanks for the review @glozow, I have addressed your comments and also made a modification to `RecursiveUpdateTxState` which will allow it to be more extensible.

⚠️ geenutts opened an issue: "Gee"
(https://github.com/bitcoin/bitcoin/issues/27439)
https://github.com/geenutts/consensus-specs/commit/e1ae7ec2ad141cf18940566d5070c1f146a7c2a9

✅ fanquake closed an issue: "Gee"
(https://github.com/bitcoin/bitcoin/issues/27439)

:lock: fanquake locked an issue: "Gee"
(https://github.com/bitcoin/bitcoin/issues/27439)

👋 pinheadmz's pull request is ready for review: "validation: implement MaybeInvalidateFork() and call from rpc getchaintips"
(https://github.com/bitcoin/bitcoin/pull/27434)

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1161121018)
The style here matches the neighboring descriptors but I see your point about using `.at(0)` -- do you think all the `descriptor::MakeScripts()` should be cleaned up like this?

Also this chunk is the patch alluded to in the comment
> This PR also patches PKDescriptor from https://github.com/bitcoin/bitcoin/pull/22051 where matching public keys were not returned.

When we expand a bare pubkey descriptor we weren't populating `out.pubkeys` and so `GetAffectedKeys()` wouldn't return anything

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1161130735)
thanks will add this -- also I think these could be `[[nodiscard]]` as well? But I don't see that for any of the other `bool Is...()` members in SPKM...

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1161130743)
Thanks, updating.

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1161130747)
ok, moving `isactive` next to `ismine`. You have a good point about legacy wallet deprecation. I suppose when that happens most of this new code will be deprecated as well since its mainly implemented only in `LegacyScriptPubKeyMan`. Would it make sense to only add the field to RPC response in legacy wallets?

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1161130750)
thanks, I do prefer!

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1161130751)
thanks got it

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1161130754)
ah really cool with `any_of` thanks

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1161130756)
okay yes

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1161130759)
ok?

> "If the address is reserved for use as a change output. It may or may not have already been used as change."

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1500927311)
@jonatack thanks so much for the great review, addressed comments (1 or 2 questions along the way) rebase on master and also rebased the GUI follow up

💬 dimitaracev commented on pull request "validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime":
(https://github.com/bitcoin/bitcoin/pull/27427#discussion_r1161135520)
IMO, it makes sense for it to to be updated to the day of the release with the assumption that the main chain does not contain any warning bits up until then. What are your thoughts?

💬 kevkevinpal commented on pull request "test: add coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27422#issuecomment-1500937726)
ok I see so this is a scenario where no command is entered. Should there also be a test for when something other than `start` `abort` or `status` is entered?