✅ maflcko closed an issue: "RFC: "Insufficient review" tag for closed PRs"
(https://github.com/bitcoin/bitcoin/issues/29839)
(https://github.com/bitcoin/bitcoin/issues/29839)
💬 maflcko commented on issue "RFC: "Insufficient review" tag for closed PRs":
(https://github.com/bitcoin/bitcoin/issues/29839#issuecomment-3455083740)
Anything left to do here?
(https://github.com/bitcoin/bitcoin/issues/29839#issuecomment-3455083740)
Anything left to do here?
💬 maflcko commented on issue "add ability to remove imported public keys/addresses (or even privkeys)":
(https://github.com/bitcoin/bitcoin/issues/23765#issuecomment-3455089395)
So I guess this can be closed?
(https://github.com/bitcoin/bitcoin/issues/23765#issuecomment-3455089395)
So I guess this can be closed?
✅ Sjors closed an issue: "Stratum v2 via IPC Mining Interface tracking issue"
(https://github.com/bitcoin/bitcoin/issues/31098)
(https://github.com/bitcoin/bitcoin/issues/31098)
💬 Sjors commented on issue "Stratum v2 via IPC Mining Interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-3455099378)
Yes, I might open a fresh tracking issue later for remaining and new interface changes. There are a few things that we want to adjust, including things that aren't urgent, but should be done when we need a breaking change anyway.
The sidecar application lives at https://github.com/Sjors/sv2-tp
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-3455099378)
Yes, I might open a fresh tracking issue later for remaining and new interface changes. There are a few things that we want to adjust, including things that aren't urgent, but should be done when we need a breaking change anyway.
The sidecar application lives at https://github.com/Sjors/sv2-tp
💬 Sammie05 commented on pull request "test: Use same rpc timeout for authproxy and cli":
(https://github.com/bitcoin/bitcoin/pull/33698#issuecomment-3455330037)
> > I went through the new changes, built the code, ran the specific test p2p_headers_sync_with_minchainwork.py --timeout-factor=0.2 in both RPC and CLI modes and everything looks good .
>
> It shouldn't look "good". They should both timeout with the same timeout value.
>
> > but why did we decide on half the timeout for CLI instead of using the same value?
>
> The goal of this pull is to use the exact same value, which is also what it does.
> > I went through the new changes, bu
...
(https://github.com/bitcoin/bitcoin/pull/33698#issuecomment-3455330037)
> > I went through the new changes, built the code, ran the specific test p2p_headers_sync_with_minchainwork.py --timeout-factor=0.2 in both RPC and CLI modes and everything looks good .
>
> It shouldn't look "good". They should both timeout with the same timeout value.
>
> > but why did we decide on half the timeout for CLI instead of using the same value?
>
> The goal of this pull is to use the exact same value, which is also what it does.
> > I went through the new changes, bu
...
💬 maflcko commented on pull request "test: Use same rpc timeout for authproxy and cli":
(https://github.com/bitcoin/bitcoin/pull/33698#issuecomment-3455485696)
> Doesn't the // 2 division mean CLI is actually getting half the RPC timeout instead of the same value?
It is explained in the trailing comment, which refers to https://github.com/bitcoin/bitcoin/blob/66667d6512294fd5dd02161b7c68c19af0865865/test/functional/test_framework/test_node.py#L322
If you have a question about a line of code, it is better to leave the question on the line of code, instead of in the general pull request thread.
(https://github.com/bitcoin/bitcoin/pull/33698#issuecomment-3455485696)
> Doesn't the // 2 division mean CLI is actually getting half the RPC timeout instead of the same value?
It is explained in the trailing comment, which refers to https://github.com/bitcoin/bitcoin/blob/66667d6512294fd5dd02161b7c68c19af0865865/test/functional/test_framework/test_node.py#L322
If you have a question about a line of code, it is better to leave the question on the line of code, instead of in the general pull request thread.
👍 willcl-ark approved a pull request: "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2"
(https://github.com/bitcoin/bitcoin/pull/33185#pullrequestreview-3387888534)
ACK 59c4898994bde3d86168075f0031c9d5a9ac5c8f
(https://github.com/bitcoin/bitcoin/pull/33185#pullrequestreview-3387888534)
ACK 59c4898994bde3d86168075f0031c9d5a9ac5c8f
💬 Sammie05 commented on pull request "test: Use same rpc timeout for authproxy and cli":
(https://github.com/bitcoin/bitcoin/pull/33698#issuecomment-3455781777)
> > Doesn't the // 2 division mean CLI is actually getting half the RPC timeout instead of the same value?
>
> It is explained in the trailing comment, which refers to
>
> https://github.com/bitcoin/bitcoin/blob/66667d6512294fd5dd02161b7c68c19af0865865/test/functional/test_framework/test_node.py#L322
>
> If you have a question about a line of code, it is better to leave the question on the line of code, instead of in the general pull request thread.
Alrighht.
(https://github.com/bitcoin/bitcoin/pull/33698#issuecomment-3455781777)
> > Doesn't the // 2 division mean CLI is actually getting half the RPC timeout instead of the same value?
>
> It is explained in the trailing comment, which refers to
>
> https://github.com/bitcoin/bitcoin/blob/66667d6512294fd5dd02161b7c68c19af0865865/test/functional/test_framework/test_node.py#L322
>
> If you have a question about a line of code, it is better to leave the question on the line of code, instead of in the general pull request thread.
Alrighht.
💬 Sjors commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3455784718)
Guix build on x86_64:
```
3de417ac1dade848d8b1609adf72c0faefcaf5acf03199259aa3aeb83e6a863f guix-build-59c4898994bd/output/aarch64-linux-gnu/SHA256SUMS.part
c6eda45fce2b34940ea53caa2acf0c1436122f89b85c0bc727834360f78a40f5 guix-build-59c4898994bd/output/aarch64-linux-gnu/bitcoin-59c4898994bd-aarch64-linux-gnu-debug.tar.gz
41584667134bfc5c35851d5005692c447d3fc529a310c7868030d6383d8bd840 guix-build-59c4898994bd/output/aarch64-linux-gnu/bitcoin-59c4898994bd-aarch64-linux-gnu.tar.gz
5c6d784b
...
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3455784718)
Guix build on x86_64:
```
3de417ac1dade848d8b1609adf72c0faefcaf5acf03199259aa3aeb83e6a863f guix-build-59c4898994bd/output/aarch64-linux-gnu/SHA256SUMS.part
c6eda45fce2b34940ea53caa2acf0c1436122f89b85c0bc727834360f78a40f5 guix-build-59c4898994bd/output/aarch64-linux-gnu/bitcoin-59c4898994bd-aarch64-linux-gnu-debug.tar.gz
41584667134bfc5c35851d5005692c447d3fc529a310c7868030d6383d8bd840 guix-build-59c4898994bd/output/aarch64-linux-gnu/bitcoin-59c4898994bd-aarch64-linux-gnu.tar.gz
5c6d784b
...
🤔 TheCharlatan reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3387750815)
Updated 20be96ee696ba8c3085da0877156e534a58c39bf -> 083814948d66aac49b6995de560a48c7889896cc ([kernelApi_76](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_76) -> [kernelApi_77](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_77), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_76..kernelApi_77))
* Addressed @stringintech's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2455010438), expand comment on purpose of the context options.
* Ad
...
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3387750815)
Updated 20be96ee696ba8c3085da0877156e534a58c39bf -> 083814948d66aac49b6995de560a48c7889896cc ([kernelApi_76](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_76) -> [kernelApi_77](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_77), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_76..kernelApi_77))
* Addressed @stringintech's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2455010438), expand comment on purpose of the context options.
* Ad
...
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468823291)
Yes! Must have missed this while re-working the way the secondary object gets passed in.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468823291)
Yes! Must have missed this while re-working the way the secondary object gets passed in.
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468816664)
We could, yes, but I think it is nice to define the interface outside of the required methods required by the template instantiation when the callbacks get wired to this notification interface. Marking these as virtual we get warnings if we don't override correctly. Alternatively we could also make this a concept, similar to the Log, but that felt a bit too complicated here. I did remove the template arguments though, those seemed needlessly confusing to me.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468816664)
We could, yes, but I think it is nice to define the interface outside of the required methods required by the template instantiation when the callbacks get wired to this notification interface. Marking these as virtual we get warnings if we don't override correctly. Alternatively we could also make this a concept, similar to the Log, but that felt a bit too complicated here. I did remove the template arguments though, those seemed needlessly confusing to me.
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468790150)
This is indeed clearer even if it introduces another copy.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468790150)
This is indeed clearer even if it introduces another copy.
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468803070)
> don't we have unsynchronized reads/writes?
Yes, we do, but with the current code I am not sure if that is easily solvable. My preferred route here is we get these changes in, and then adapt our internal logging code to better cater to the kernel usages. There are other problems with it too: We can't distinguish between different logging sources and the global is potentially leaky on exit.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468803070)
> don't we have unsynchronized reads/writes?
Yes, we do, but with the current code I am not sure if that is easily solvable. My preferred route here is we get these changes in, and then adapt our internal logging code to better cater to the kernel usages. There are other problems with it too: We can't distinguish between different logging sources and the global is potentially leaky on exit.
✅ antonilol closed an issue: "add ability to remove imported public keys/addresses (or even privkeys)"
(https://github.com/bitcoin/bitcoin/issues/23765)
(https://github.com/bitcoin/bitcoin/issues/23765)
🚀 fanquake merged a pull request: "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2"
(https://github.com/bitcoin/bitcoin/pull/33185)
(https://github.com/bitcoin/bitcoin/pull/33185)
🤔 cedwies reviewed a pull request: "validation: do not wipe utxo cache for stats/scans/snapshots"
(https://github.com/bitcoin/bitcoin/pull/33680#pullrequestreview-3387641073)
Approach ACK
Do `ForceFlushToDisk(...)`/`FlushToDisk(...)` require `cs_main` to be held?
Most call sites hold it during shutdown flushes, but the function itself doesn’t assert or document that (no `AssertLockHeld(cs_main)`). If this is correct, I think a short comment can help that a Lock is not strictly needed.
(https://github.com/bitcoin/bitcoin/pull/33680#pullrequestreview-3387641073)
Approach ACK
Do `ForceFlushToDisk(...)`/`FlushToDisk(...)` require `cs_main` to be held?
Most call sites hold it during shutdown flushes, but the function itself doesn’t assert or document that (no `AssertLockHeld(cs_main)`). If this is correct, I think a short comment can help that a Lock is not strictly needed.
💬 cedwies commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2468910777)
Isn't `LogPrintf` deprecated? Could we replace this with `LogError` or `LogWarning`? Maybe also including the flush mode can be helpful.
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2468910777)
Isn't `LogPrintf` deprecated? Could we replace this with `LogError` or `LogWarning`? Maybe also including the flush mode can be helpful.
💬 cedwies commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2468702094)
Why are we wiping the cache here? We only compute stats from `CoinsDB()`, not the in-memory cache, so a non-wiping flush (`wipe_cache=false`) should be enough for consistency (?)
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2468702094)
Why are we wiping the cache here? We only compute stats from `CoinsDB()`, not the in-memory cache, so a non-wiping flush (`wipe_cache=false`) should be enough for consistency (?)