š¬ ismaelsadeeq commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482293453)
Thanks will do when retouching.
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482293453)
Thanks will do when retouching.
š¬ ismaelsadeeq commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482303861)
Iām a bit on the fence here a referenced parameter already signals that the value comes from elsewhere, also prevents nullptr input (must be initialized).
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482303861)
Iām a bit on the fence here a referenced parameter already signals that the value comes from elsewhere, also prevents nullptr input (must be initialized).
š¬ ryanofsky commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3474302360)
Code review ACK 6eaa00fe20206baedc0d8ade5bb8d066ea615704. Just documentation updates and test clarifications since last review, also splitting up a commit.
---
re: https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3472031005
> > and I think BlockTemplate accessors could be added so miners do not need to make hardcoded assumptions about the coinbase transaction? ([#33745 (comment)](https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477980297)).
>
> The Template Prov
...
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3474302360)
Code review ACK 6eaa00fe20206baedc0d8ade5bb8d066ea615704. Just documentation updates and test clarifications since last review, also splitting up a commit.
---
re: https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3472031005
> > and I think BlockTemplate accessors could be added so miners do not need to make hardcoded assumptions about the coinbase transaction? ([#33745 (comment)](https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477980297)).
>
> The Template Prov
...
š¤ ismaelsadeeq reviewed a pull request: "mining: check witness commitment in submitBlock"
(https://github.com/bitcoin/bitcoin/pull/33745#pullrequestreview-3405553194)
Concept ACK, will review.
(https://github.com/bitcoin/bitcoin/pull/33745#pullrequestreview-3405553194)
Concept ACK, will review.
š¬ Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482334128)
I also don't think a pointer is much more clear than a reference.
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482334128)
I also don't think a pointer is much more clear than a reference.
š¬ furszy commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482352743)
Probably providing a "wait context" object (which could be a shared pointer) would be slightly more appropriate, as it clearly indicates that it is specifically used in this workflow. Rather than a ref bool which is not so clear.
Still, we might be overthinking this. I think we can continue as is for now.
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482352743)
Probably providing a "wait context" object (which could be a shared pointer) would be slightly more appropriate, as it clearly indicates that it is specifically used in this workflow. Rather than a ref bool which is not so clear.
Still, we might be overthinking this. I think we can continue as is for now.
š¬ ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3474397434)
Thanks for you recent comment @ryanofsky @Sjors
> Is this the plan for the current PR? Otherwise maybe it makes more sense to move that to the tracking issue.
> This all seems reasonable and is helpful. I do agree with Sjors though it could be good to move the information about next steps into https://github.com/bitcoin/bitcoin/issues/33389 which could become a tracking issue. Or you could close https://github.com/bitcoin/bitcoin/issues/33389 and make a new tracking issue (if things have
...
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3474397434)
Thanks for you recent comment @ryanofsky @Sjors
> Is this the plan for the current PR? Otherwise maybe it makes more sense to move that to the tracking issue.
> This all seems reasonable and is helpful. I do agree with Sjors though it could be good to move the information about next steps into https://github.com/bitcoin/bitcoin/issues/33389 which could become a tracking issue. Or you could close https://github.com/bitcoin/bitcoin/issues/33389 and make a new tracking issue (if things have
...
š¬ polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2482397743)
Thanks for the suggestion, yep it is cleaner. Now should be as easy as changing the default in the arg definition.
btw: I used `self.Arg<bool>`, I think it makes more sense, `self.MaybeArg<bool>` is defined for optional params with no defaults.
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2482397743)
Thanks for the suggestion, yep it is cleaner. Now should be as easy as changing the default in the arg definition.
btw: I used `self.Arg<bool>`, I think it makes more sense, `self.MaybeArg<bool>` is defined for optional params with no defaults.
š¬ mzumsande commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482405261)
linter complains about trailing whitespace (in both commits)
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482405261)
linter complains about trailing whitespace (in both commits)
š glozow merged a pull request: "refactor/doc: Add blockman param to GetTransaction doc comment"
(https://github.com/bitcoin/bitcoin/pull/33683)
(https://github.com/bitcoin/bitcoin/pull/33683)
š¤ glozow reviewed a pull request: "node: change a tx-relay on/off flag to enum"
(https://github.com/bitcoin/bitcoin/pull/33567#pullrequestreview-3405305256)
utACK 07a926474b5a6fa1d3d4656362a0117611f6da2f
(https://github.com/bitcoin/bitcoin/pull/33567#pullrequestreview-3405305256)
utACK 07a926474b5a6fa1d3d4656362a0117611f6da2f
š¬ glozow commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2482173040)
nit: "what" is a pretty unhelpful variable name
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2482173040)
nit: "what" is a pretty unhelpful variable name
š¬ glozow commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2482168470)
Awkward part about splitting this off: adding it to mempool is not actually optional at this point in time.
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2482168470)
Awkward part about splitting this off: adding it to mempool is not actually optional at this point in time.
š glozow merged a pull request: "node: change a tx-relay on/off flag to enum"
(https://github.com/bitcoin/bitcoin/pull/33567)
(https://github.com/bitcoin/bitcoin/pull/33567)
š¬ ajtowns commented on pull request "miner: don't needlessly append dummy OP_0 to bip34":
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-3474555715)
> I also don't think `CBlockTemplate` should have an `extraNonce`.
I think a **block** should (almost) always have an extraNonce -- whether for mainnet to provide sufficient work, or for early blocks on regtest/signet to avoid bad-cb-length, or for test net blocks to more closely simulate mainnet behaviour. Whether the template should also include an extraNonce depends on the user of the template -- if they want a template they can just apply work to via nNonce, without further fussing about
...
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-3474555715)
> I also don't think `CBlockTemplate` should have an `extraNonce`.
I think a **block** should (almost) always have an extraNonce -- whether for mainnet to provide sufficient work, or for early blocks on regtest/signet to avoid bad-cb-length, or for test net blocks to more closely simulate mainnet behaviour. Whether the template should also include an extraNonce depends on the user of the template -- if they want a template they can just apply work to via nNonce, without further fussing about
...
š¬ glozow commented on issue "test: break `feature_block` into subtests?":
(https://github.com/bitcoin/bitcoin/issues/32877#issuecomment-3474586758)
Have we considered giving functional tests severity ratings, like with bench? Some tests take longer because they cover things that are harder to reach. We also have some things that go on timers and it would be nice to not mock the times all the time, but it's annoying if they are bottlenecks.
(https://github.com/bitcoin/bitcoin/issues/32877#issuecomment-3474586758)
Have we considered giving functional tests severity ratings, like with bench? Some tests take longer because they cover things that are harder to reach. We also have some things that go on timers and it would be nice to not mock the times all the time, but it's annoying if they are bottlenecks.
š¬ ajtowns commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474594418)
If you're deliberately changing the standardness rules specified in BIP143, that should be in the PR description, and possibly proposed as an update to the BIP. If you're not deliberately changing them, they shouldn't be changed by accident. Personally, I don't see the justification for changing them here, though I don't really understand why it was specced as a policy rule rather than a consensus one at the time, either.
Personally, I don't really understand the motivation for reconsidering
...
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474594418)
If you're deliberately changing the standardness rules specified in BIP143, that should be in the PR description, and possibly proposed as an update to the BIP. If you're not deliberately changing them, they shouldn't be changed by accident. Personally, I don't see the justification for changing them here, though I don't really understand why it was specced as a policy rule rather than a consensus one at the time, either.
Personally, I don't really understand the motivation for reconsidering
...
š¬ roconnor-blockstream commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474639402)
I guess we could change BIP143. Currently the text reads:
> Each public key passed to a sigop inside version 0 witness program must be a compressed key: the first byte MUST be either 0x02 or 0x03, and the size MUST be 33 bytes. Transactions that break this rule will not be relayed or mined by default.
Note that is not how Bitcoin Core implements `SCRIPT_VERIFY_WITNESS_PUBKEYTYPE` currently as you can pass pubkeys to CHECKMULTISIG that violate this policy when the signature stack is used
...
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474639402)
I guess we could change BIP143. Currently the text reads:
> Each public key passed to a sigop inside version 0 witness program must be a compressed key: the first byte MUST be either 0x02 or 0x03, and the size MUST be 33 bytes. Transactions that break this rule will not be relayed or mined by default.
Note that is not how Bitcoin Core implements `SCRIPT_VERIFY_WITNESS_PUBKEYTYPE` currently as you can pass pubkeys to CHECKMULTISIG that violate this policy when the signature stack is used
...
š¬ da1sychain commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482509366)
Thank you, good idea. Added some clarifying language to that sentence.
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482509366)
Thank you, good idea. Added some clarifying language to that sentence.
š¬ darosior commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#issuecomment-3474716867)
> See #33498.
This is only one such vector. It's good to have a warning until we can be reasonably confident that it's not trivially possible to link two different network addresses of a node. I am also skeptical we could ever reach this level of confidence.
(https://github.com/bitcoin/bitcoin/pull/33750#issuecomment-3474716867)
> See #33498.
This is only one such vector. It's good to have a warning until we can be reasonably confident that it's not trivially possible to link two different network addresses of a node. I am also skeptical we could ever reach this level of confidence.