:lock: fanquake locked an issue: "Gee"
(https://github.com/bitcoin/bitcoin/issues/27439)
(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)
(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
(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...
(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.
(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?
(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!
(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
(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
(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
(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."
(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
(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?
(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?
(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?
💬 hebasto commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1500980698)
Updated f1370b2c1586f7fe487d9f17ee53bcd9b87a9f23 -> d462e3da7fc6be75269e88928fd80fc98c405474 ([pr26762.05](https://github.com/hebasto/bitcoin/commits/pr26762.05) -> [pr26762.07](https://github.com/hebasto/bitcoin/commits/pr26762.07), [diff](https://github.com/hebasto/bitcoin/compare/pr26762.05..pr26762.07)):
- addressed @martinus's comments
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1500980698)
Updated f1370b2c1586f7fe487d9f17ee53bcd9b87a9f23 -> d462e3da7fc6be75269e88928fd80fc98c405474 ([pr26762.05](https://github.com/hebasto/bitcoin/commits/pr26762.05) -> [pr26762.07](https://github.com/hebasto/bitcoin/commits/pr26762.07), [diff](https://github.com/hebasto/bitcoin/compare/pr26762.05..pr26762.07)):
- addressed @martinus's comments
💬 hebasto commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1161160170)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1500980698).
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1161160170)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1500980698).
💬 hebasto commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1161160203)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1500980698).
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1161160203)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1500980698).
💬 hebasto commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1161160213)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1500980698).
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1161160213)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1500980698).
💬 ismaelsadeeq commented on pull request "test: add coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27422#issuecomment-1500982428)
Yes can be something like
`assert_raises_rpc_error(-8, "Invalid action 'word'", self.nodes[0].scantxoutset, "word")`
(https://github.com/bitcoin/bitcoin/pull/27422#issuecomment-1500982428)
Yes can be something like
`assert_raises_rpc_error(-8, "Invalid action 'word'", self.nodes[0].scantxoutset, "word")`
🤔 kouloumos reviewed a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-1376804705)
<details><summary> I've measured the number of executed instructions for each tracepoint based on the current and new implementation and verified that the overhead for tracepoints' arguments preparation - when enabled but not hooked - has been eliminated. Click to expand for details. </summary>
<br/>
master (49b87bfe7e2799d25ce709123ecafa872b36e87a)
Tracepoint|Disabled|Enabled (not hooked)|Enabled (hooked)
:---|:---|:---|:---
mempool:added|1|24|24
mempool:removed|1|136|136
mempool:rep
...
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-1376804705)
<details><summary> I've measured the number of executed instructions for each tracepoint based on the current and new implementation and verified that the overhead for tracepoints' arguments preparation - when enabled but not hooked - has been eliminated. Click to expand for details. </summary>
<br/>
master (49b87bfe7e2799d25ce709123ecafa872b36e87a)
Tracepoint|Disabled|Enabled (not hooked)|Enabled (hooked)
:---|:---|:---|:---
mempool:added|1|24|24
mempool:removed|1|136|136
mempool:rep
...