💬 Sjors commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-3473947623)
Possibly relevant: #33756
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-3473947623)
Possibly relevant: #33756
💬 Sjors commented on issue "RFC: Bitcoin Core Node `BlockTemplateManager`":
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3473950791)
Do you think #33756 would be compatible with this?
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3473950791)
Do you think #33756 would be compatible with this?
💬 A-Manning commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2482073588)
Applied the patch, thanks
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2482073588)
Applied the patch, thanks
💬 A-Manning commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2482074480)
Done
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2482074480)
Done
💬 ryanofsky commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482074676)
re: https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481903500
> It's somewhat unintuitive that we're waiting for a local variable to change. It might be more clear if we make a trivial struct `TemplateNotifications` and have this line check `template_notifications.m_interrupt_wait` instead. It's more clear that something called "notifications" can change from under us.
I feel like it's not that confusing once you notice it's declared as outside reference (`bool& interrupt_wait`)
...
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482074676)
re: https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481903500
> It's somewhat unintuitive that we're waiting for a local variable to change. It might be more clear if we make a trivial struct `TemplateNotifications` and have this line check `template_notifications.m_interrupt_wait` instead. It's more clear that something called "notifications" can change from under us.
I feel like it's not that confusing once you notice it's declared as outside reference (`bool& interrupt_wait`)
...
💬 Fi3 commented on issue "Header-only support for waitNext()":
(https://github.com/bitcoin/bitcoin/issues/33756#issuecomment-3473971497)
it make sense for the TP to optimistically create templates as soon as there is a possible new block since this will reduce the time that a miner waste on stale jobs. Is kinda safe since the TP can stop following the optimistic path when 2 or more consecutive invalid blocks are received. If we leave it like it is now the miner will keep mine on the old job until the block template do not provide a new job.
So worst case we waste a bunch of hundreds milliseconds mining invalid jobs, that we are
...
(https://github.com/bitcoin/bitcoin/issues/33756#issuecomment-3473971497)
it make sense for the TP to optimistically create templates as soon as there is a possible new block since this will reduce the time that a miner waste on stale jobs. Is kinda safe since the TP can stop following the optimistic path when 2 or more consecutive invalid blocks are received. If we leave it like it is now the miner will keep mine on the old job until the block template do not provide a new job.
So worst case we waste a bunch of hundreds milliseconds mining invalid jobs, that we are
...
💬 0xB10C commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3473985400)
Note that the @plebhash comments were actually remove from the archive (as it just pulls the GitHub API. Gone from the GitHub API means gone from the latest version of the archive and mirroring) and might thus be missing from the mirroring in the next rebuild..
See https://github.com/bitcoin-data/github-metadata-backup-bitcoin-bitcoin/commit/9ca6ff3e35249f8ccb710de7cee6ff13e028f0ee#diff-a4bfd29817b2ed302dbaf2c9bd4514ace8aa1ffeacf02046400d93b526b8ab02L847
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3473985400)
Note that the @plebhash comments were actually remove from the archive (as it just pulls the GitHub API. Gone from the GitHub API means gone from the latest version of the archive and mirroring) and might thus be missing from the mirroring in the next rebuild..
See https://github.com/bitcoin-data/github-metadata-backup-bitcoin-bitcoin/commit/9ca6ff3e35249f8ccb710de7cee6ff13e028f0ee#diff-a4bfd29817b2ed302dbaf2c9bd4514ace8aa1ffeacf02046400d93b526b8ab02L847
💬 glozow commented on pull request "docs: clarify RPC credentials security boundary":
(https://github.com/bitcoin/bitcoin/pull/33196#discussion_r2482100129)
The first few sentences seem largely redundant with the rest of the docs (see few paragraphs of "Security" section, and "Secure authentication". However, the bit about whitelisting RPC commands for specific users seems helpful.
(https://github.com/bitcoin/bitcoin/pull/33196#discussion_r2482100129)
The first few sentences seem largely redundant with the rest of the docs (see few paragraphs of "Security" section, and "Secure authentication". However, the bit about whitelisting RPC commands for specific users seems helpful.
📝 l0rinc opened a pull request: "refactor: make script Solver's often-unused solutions parameter optional"
(https://github.com/bitcoin/bitcoin/pull/33757)
### Summary
Many callsites only need to determine the script type and don't use the parsed solutions.
Previously, these callers still incurred the cost of allocating and populating a `std::vector<std::vector<unsigned char>>`.
### Fix
Changing `Solver` signature from reference to nullable pointer parameter with `nullptr` default allows calling the method for its result only. Note that we can't call `std::optional` here because that would copy the values but we need the result to be provided
...
(https://github.com/bitcoin/bitcoin/pull/33757)
### Summary
Many callsites only need to determine the script type and don't use the parsed solutions.
Previously, these callers still incurred the cost of allocating and populating a `std::vector<std::vector<unsigned char>>`.
### Fix
Changing `Solver` signature from reference to nullable pointer parameter with `nullptr` default allows calling the method for its result only. Note that we can't call `std::optional` here because that would copy the values but we need the result to be provided
...
💬 l0rinc commented on pull request "refactor: optimize: avoid allocations in script & policy verification":
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2482103559)
I don't like the reuses, seems like a workaround instead of a fix, pushed an alternative to https://github.com/bitcoin/bitcoin/pull/33757, added @Raimo33 as a coauthor
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2482103559)
I don't like the reuses, seems like a workaround instead of a fix, pushed an alternative to https://github.com/bitcoin/bitcoin/pull/33757, added @Raimo33 as a coauthor
💬 davidgumberg commented on pull request "ci: gha: Set debug_pull_request_number_str annotation":
(https://github.com/bitcoin/bitcoin/pull/33754#issuecomment-3474017856)
lgtm ack https://github.com/bitcoin/bitcoin/commit/fa9d0f994b45a94e3f26c01e395c58ff59f47f43
https://github.com/bitcoin/bitcoin/actions/runs/18971691417/job/54180745411?pr=33754#step:2:11
(https://github.com/bitcoin/bitcoin/pull/33754#issuecomment-3474017856)
lgtm ack https://github.com/bitcoin/bitcoin/commit/fa9d0f994b45a94e3f26c01e395c58ff59f47f43
https://github.com/bitcoin/bitcoin/actions/runs/18971691417/job/54180745411?pr=33754#step:2:11
💬 Raimo33 commented on pull request "refactor: optimize: avoid allocations in script & policy verification":
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2482120044)
what about the other commits? @l0rinc.
there are some intresting zero-cost optimizations. maybe not worth for a PR alone, but could be included as small refactorings in bigger PRs.
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2482120044)
what about the other commits? @l0rinc.
there are some intresting zero-cost optimizations. maybe not worth for a PR alone, but could be included as small refactorings in bigger PRs.
💬 Raimo33 commented on pull request "refactor: make script Solver's often-unused solutions parameter optional":
(https://github.com/bitcoin/bitcoin/pull/33757#issuecomment-3474029652)
Code Review ACK 2726abd896817c0dfd5b171a643bad6af015d070
I proposed the same in this private commit: 3ea121d49c01935f74673cbb825ed608179e43d5
would like a bit more clarity on the benchmarks referenced. are we talking about `CCoinsCaching`?
(https://github.com/bitcoin/bitcoin/pull/33757#issuecomment-3474029652)
Code Review ACK 2726abd896817c0dfd5b171a643bad6af015d070
I proposed the same in this private commit: 3ea121d49c01935f74673cbb825ed608179e43d5
would like a bit more clarity on the benchmarks referenced. are we talking about `CCoinsCaching`?
💬 ajtowns commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474035674)
This change affects enforcement of `SCRIPT_VERIFY_WITNESS_PUBKEYTYPE` which afaics should not be weakened [bip143](https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki#restrictions-on-public-key-type)?
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474035674)
This change affects enforcement of `SCRIPT_VERIFY_WITNESS_PUBKEYTYPE` which afaics should not be weakened [bip143](https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki#restrictions-on-public-key-type)?
🚀 glozow merged a pull request: "[IBD] coins: increase default UTXO flush batch size to 32 MiB"
(https://github.com/bitcoin/bitcoin/pull/31645)
(https://github.com/bitcoin/bitcoin/pull/31645)
💬 ismaelsadeeq commented on pull request "node: add `BlockTemplate{Cache, Manager}`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2482188424)
This is gone now.
(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
...
(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
...
(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.
(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
...
(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)
(https://github.com/bitcoin/bitcoin/issues/33389)