š¬ ismaelsadeeq commented on issue "RFC: Bitcoin Core Node `BlockTemplateManager`":
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3474206427)
> Do you think https://github.com/bitcoin/bitcoin/issues/33756 would be compatible with this?
I have not take a look at that yet, I will do and revert thanks.
Closing this in favour of https://github.com/bitcoin/bitcoin/issues/33758 discussion can continue though.
Thanks for engaging.
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3474206427)
> Do you think https://github.com/bitcoin/bitcoin/issues/33756 would be compatible with this?
I have not take a look at that yet, I will do and revert thanks.
Closing this in favour of https://github.com/bitcoin/bitcoin/issues/33758 discussion can continue though.
Thanks for engaging.
š¬ l0rinc commented on pull request "refactor: make script Solver's often-unused solutions parameter optional":
(https://github.com/bitcoin/bitcoin/pull/33757#issuecomment-3474213420)
> would like a bit more clarity on the benchmarks referenced
Was waiting for corecheck to finish, but it shows only `2.63%` speedup for `CCoinsCaching`.
I'll measure it properly, but in the meantime I'll leave this as a refactoring only since the resulting code is arguably cleaner.
(https://github.com/bitcoin/bitcoin/pull/33757#issuecomment-3474213420)
> would like a bit more clarity on the benchmarks referenced
Was waiting for corecheck to finish, but it shows only `2.63%` speedup for `CCoinsCaching`.
I'll measure it properly, but in the meantime I'll leave this as a refactoring only since the resulting code is arguably cleaner.
š l0rinc's pull request is ready for review: "refactor: make script Solver's often-unused solutions parameter optional"
(https://github.com/bitcoin/bitcoin/pull/33757)
(https://github.com/bitcoin/bitcoin/pull/33757)
š¬ ismaelsadeeq commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482289948)
Yes.
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482289948)
Yes.
š¬ 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.