💬 ismaelsadeeq commented on pull request "node: add `BlockTemplate{Cache, Manager}`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2482188424)
This is gone now.

💬 ismaelsadeeq commented on pull request "node: add `BlockTemplate{Cache, Manager}`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3474113647)
Forced pushed from 5f798b4a3b1a978b8ac954db54e9e8dfc7da6319 to 084bfbc1ec7f8f64f54d231bb641285622311b59 Compare diff [dfc7da6319..084bfbc1](https://github.com/bitcoin/bitcoin/compare/5f798b4a3b1a978b8ac954db54e9e8dfc7da6319..084bfbc1ec7f8f64f54d231bb641285622311b59)

Thanks for your thorough review. I've made significant changes following your feedback:

* Removed the first commit, as it is out of scope for now. [https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2379497293](https://g
...

💬 roconnor-blockstream commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474132937)
@ajtowns I did consider that.

Firstly we should note that `SCRIPT_VERIFY_WITNESS_PUBKEYTYPE` and BIP-143 rules regarding compressed public keys are not consensus rules, but policy only. So at least it *can* be weakened.

Regarding as to whether it should be weakened: Currently Bitcoin Core's implementation choice for CHECKMULTISIG is such that once enough signatures match the pubkeys, no further pubkeys in the CHECKMULTISIG stack are inspected for well-formedness policy constraints. That
...

💬 ismaelsadeeq commented on pull request "node: add `BlockTemplate{Cache, Manager}`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2482211458)
Yes this will be good for precision but In the current iteration of the PR I switched to using the exact block assembly time as save in the block `nTime` which is in seconds.

However I took the other suggestion of changing the name to maximum age.

⚠️ ismaelsadeeq opened an issue: "BlockTemplate Manager Tracking issue"
(https://github.com/bitcoin/bitcoin/issues/33758)
The main motivation and design rationale are discussed in #33389.

What to review

* #33421

### Plan

* [ ] **Introduce the Block Template Cache (Manager) #33421**
Implement a cache layer. All newly created block templates should be stored along with their respective configuration options. Client requests for block templates will specify the **maximum age** of the template in seconds; that is, how fresh they want the template to be:

* If a template with matching options exists in the cache
...

✅ ismaelsadeeq closed an issue: "RFC: Bitcoin Core Node `BlockTemplateManager`"
(https://github.com/bitcoin/bitcoin/issues/33389)

💬 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.

💬 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.

👋 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)

💬 ismaelsadeeq commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(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.

💬 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).

💬 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
...

🤔 ismaelsadeeq reviewed a pull request: "mining: check witness commitment in submitBlock"
(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.

💬 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.

💬 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
...

💬 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.

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

🚀 glozow merged a pull request: "refactor/doc: Add blockman param to GetTransaction doc comment"
(https://github.com/bitcoin/bitcoin/pull/33683)