🤔 cedwies reviewed a pull request: "validation: do not wipe utxo cache for stats/scans/snapshots"
(https://github.com/bitcoin/bitcoin/pull/33680#pullrequestreview-3531563691)
reACK 14abac1
(https://github.com/bitcoin/bitcoin/pull/33680#pullrequestreview-3531563691)
reACK 14abac1
💬 ryanofsky commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3603399341)
re: https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602538081
> Makes sense. We'll have to find another method of preventing a burst of new templates while the node churns through the last day worth of blocks.
I think the fact that we have separate `createNewBlock` and `waitNext` to functions to fetch the initial block template and subsequent ones should make this easier to solve, because `createNewBlock` can be conservative and wait to return until after some cooldown period, an
...
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3603399341)
re: https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602538081
> Makes sense. We'll have to find another method of preventing a burst of new templates while the node churns through the last day worth of blocks.
I think the fact that we have separate `createNewBlock` and `waitNext` to functions to fetch the initial block template and subsequent ones should make this easier to solve, because `createNewBlock` can be conservative and wait to return until after some cooldown period, an
...
💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2582425482)
Since `-blockreservedweight` has a default value, I don't think the absence of this setting should behave any different from when the user manually sets it to the default. This makes that intention clear.
It also makes the transition to `ApplyMiningDefaults` in https://github.com/bitcoin/bitcoin/pull/33966/commits/5f3e67efddd2a8a8743bb19765a5b38755d3f460 simpler, because `default_block_max_weight` and `default_block_reserved_weight` on `MiningArgs` are _not_ optionals. That's for the same rea
...
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2582425482)
Since `-blockreservedweight` has a default value, I don't think the absence of this setting should behave any different from when the user manually sets it to the default. This makes that intention clear.
It also makes the transition to `ApplyMiningDefaults` in https://github.com/bitcoin/bitcoin/pull/33966/commits/5f3e67efddd2a8a8743bb19765a5b38755d3f460 simpler, because `default_block_max_weight` and `default_block_reserved_weight` on `MiningArgs` are _not_ optionals. That's for the same rea
...
💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2582434748)
I took the suggested message.
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2582434748)
I took the suggested message.
💬 Sjors commented on issue "Mining IPC `createNewBlock` should not return before IBD is over":
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3603530038)
@ryanofsky wrote:
> `GetTime() - last_block_connected_time < COOLDOWN`
@jbesraa:
That looks nice, and would keep client code simple.
> We run both sv2-tp(by sjors, version 1.0.4)
You should upgrade to [v1.0.5](https://github.com/stratum-mining/sv2-tp/releases/tag/v1.0.5) :-)
> and core 30
At this point I think it's better to run the `30.x` branch built from source if you can. It has quite a few IPC related bug fixes. Otherwise you're going to hunt for bugs that have already been fixed. On
...
(https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3603530038)
@ryanofsky wrote:
> `GetTime() - last_block_connected_time < COOLDOWN`
@jbesraa:
That looks nice, and would keep client code simple.
> We run both sv2-tp(by sjors, version 1.0.4)
You should upgrade to [v1.0.5](https://github.com/stratum-mining/sv2-tp/releases/tag/v1.0.5) :-)
> and core 30
At this point I think it's better to run the `30.x` branch built from source if you can. It has quite a few IPC related bug fixes. Otherwise you're going to hunt for bugs that have already been fixed. On
...
💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3603546302)
push to 22ec2f95eecddd5dc6ea53f670f232a7e425a8d0:
- fix conflicts with master, which were all logging fixes from https://github.com/bitcoin/bitcoin/pull/33960
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3603546302)
push to 22ec2f95eecddd5dc6ea53f670f232a7e425a8d0:
- fix conflicts with master, which were all logging fixes from https://github.com/bitcoin/bitcoin/pull/33960
👍 hodlinator approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3531762528)
re-ACK 6826375e85a690d583cdeae3ff68d8703b1802ca
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3531762528)
re-ACK 6826375e85a690d583cdeae3ff68d8703b1802ca
💬 l0rinc commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582602195)
there's a **lot** happening in a single commit - please consider splitting the low risk changes such as this line from introducing a new feature or a doc change. It would help guide the reviewers if it were split into trivial-to-review chunks where the commit message explains the thinking.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582602195)
there's a **lot** happening in a single commit - please consider splitting the low risk changes such as this line from introducing a new feature or a doc change. It would help guide the reviewers if it were split into trivial-to-review chunks where the commit message explains the thinking.
👍 sedited approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3531852343)
Re-ACK 6826375e85a690d583cdeae3ff68d8703b1802ca
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3531852343)
Re-ACK 6826375e85a690d583cdeae3ff68d8703b1802ca
💬 sedited commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582578921)
Nit: It would be good to assert that we don't fall through here, as per the [developer-notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures), by adding an `assert(false)` here.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582578921)
Nit: It would be good to assert that we don't fall through here, as per the [developer-notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures), by adding an `assert(false)` here.
💬 sedited commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582661911)
Nit: I don't like that we are returning a nullopt for a success code here. If we'd finally have #25665 we could have a proper result type here. Leaving as a nit, because I don't have a good alternative either.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582661911)
Nit: I don't like that we are returning a nullopt for a success code here. If we'd finally have #25665 we could have a proper result type here. Leaving as a nit, because I don't have a good alternative either.
💬 sedited commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582597724)
Nit: I think the extra `are` is a typo.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582597724)
Nit: I think the extra `are` is a typo.
💬 hebasto commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3603896660)
> If that is the case, perhaps then instead of introducing more opinionated fallback heirarchies we should look at re-working how toolchains are defined more holistically? I don't see an obvious/clean approach not involving two sets of env vars though...
I agree that the depends build subsystem should accept two sets of variables: one for host/target-specific tools and another for native tools.
The naming for the former is well established (`CC`, `CXX`, etc) and it makes no senses to use a
...
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3603896660)
> If that is the case, perhaps then instead of introducing more opinionated fallback heirarchies we should look at re-working how toolchains are defined more holistically? I don't see an obvious/clean approach not involving two sets of env vars though...
I agree that the depends build subsystem should accept two sets of variables: one for host/target-specific tools and another for native tools.
The naming for the former is well established (`CC`, `CXX`, etc) and it makes no senses to use a
...
👍 sedited approved a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-2144994913)
Re-ACK f0aff63b5ad51566e626d5b24eee08eb81df54a1
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-2144994913)
Re-ACK f0aff63b5ad51566e626d5b24eee08eb81df54a1
💬 sedited commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1656917172)
Style nit: `{` on new line.
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1656917172)
Style nit: `{` on new line.
💬 sedited commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1656947427)
Nit: Call this DstContructed like in the template declaration?
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1656947427)
Nit: Call this DstContructed like in the template declaration?
💬 sedited commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3603908712)
Re https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3597867646
> I'd expect both classes to be useful: Result class for higher level code, std::expected for low-level code.
I think I'd be fine with that, but would appreciate to just have a consistent way to propagate our errors. I was more suggesting it, because it seems like this PR is not compelling for everybody. But maybe this can get over the line now :)
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3603908712)
Re https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3597867646
> I'd expect both classes to be useful: Result class for higher level code, std::expected for low-level code.
I think I'd be fine with that, but would appreciate to just have a consistent way to propagate our errors. I was more suggesting it, because it seems like this PR is not compelling for everybody. But maybe this can get over the line now :)
💬 ryanofsky commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3604081211)
Following up on https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3578402033, I added log statements to the `BlockTemplateImpl` constructor and destructor and wrote a python test creating a block template object and calling the destroy method or not (`DESTROY` variable), and deleting the python template variable or not (`DEL` variable). As soon as either of these things were done, I could see the `~BlockTemplateImpl` destructor being called. I could also see the destructor being calle
...
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3604081211)
Following up on https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3578402033, I added log statements to the `BlockTemplateImpl` constructor and destructor and wrote a python test creating a block template object and calling the destroy method or not (`DESTROY` variable), and deleting the python template variable or not (`DEL` variable). As soon as either of these things were done, I could see the `~BlockTemplateImpl` destructor being called. I could also see the destructor being calle
...
💬 ryanofsky commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3604126370)
re: https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3582775203
> after block [`0000000000000000000077e34472247575f4858357541269d56a8df5b429862b`](https://mempool.space/block/0000000000000000000077e34472247575f4858357541269d56a8df5b429862b), we destroyed 84 stale templates:
> [...]
> all of this come from the execution of [`TemplateData::destroy_ipc_client`](https://github.com/stratum-mining/sv2-apps/blob/ecf5f258766ac9e08bd8aaaa53b45b1c1bf961d8/bitcoin-core-sv2/src/template_data.rs
...
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3604126370)
re: https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3582775203
> after block [`0000000000000000000077e34472247575f4858357541269d56a8df5b429862b`](https://mempool.space/block/0000000000000000000077e34472247575f4858357541269d56a8df5b429862b), we destroyed 84 stale templates:
> [...]
> all of this come from the execution of [`TemplateData::destroy_ipc_client`](https://github.com/stratum-mining/sv2-apps/blob/ecf5f258766ac9e08bd8aaaa53b45b1c1bf961d8/bitcoin-core-sv2/src/template_data.rs
...
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582956040)
> If we'd finally have https://github.com/bitcoin/bitcoin/pull/25665 we could have a proper result type here.
Agree - but maybe meanwhile we can use the following?
https://github.com/bitcoin/bitcoin/compare/6826375e85a690d583cdeae3ff68d8703b1802ca..f07c765aa6bdba2511ceec56aa7f9755fa29a81e
(changed the return type back to be boolean + added an output parameter for the error enum)
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582956040)
> If we'd finally have https://github.com/bitcoin/bitcoin/pull/25665 we could have a proper result type here.
Agree - but maybe meanwhile we can use the following?
https://github.com/bitcoin/bitcoin/compare/6826375e85a690d583cdeae3ff68d8703b1802ca..f07c765aa6bdba2511ceec56aa7f9755fa29a81e
(changed the return type back to be boolean + added an output parameter for the error enum)