💬 kevkevinpal commented on pull request "test: add coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27422#issuecomment-1500649484)
utACK by visually looking at the code I can see 3 different options
`status` `abort` and `start` which requires 1 argument

but if it doesn't get any of the above options it calls `JSONRPCError` link below
[Link to blockchain.cpp Line #2243](https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L2243)

💬 ismaelsadeeq commented on pull request "test: add coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27422#issuecomment-1500679698)
Thank you @kevkevinpal Yes if it gets an argument that is not `status` `abort` or `start`
[ blockchain.cpp#L2243 ](https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L2243) is called.

But
` scantxoutset `action argument is specified as
RPCArg::Optional::NO [blockchain.cpp#L2062](https://github.com/bitcoin/bitcoin/blob/5a8bd4505687a7ec76d731b1a8249ee04d641990/src/rpc/blockchain.cpp#L2062).
If there are no arguments scantxoutset won't be called at all.
It returns the
...

💬 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_r1161062770)
I've updated it to this, this is much more clear.

💬 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_r1161062811)
I've added a check for this.

💬 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_r1161062833)
Done

💬 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