💬 gmaxwell commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3486413945)
@luke-jr Can you point out where your filtering of recent versions was discussed in public in advance of implementation (per the policy, e.g. "Behavior outside of these expectations may be reasonable in some situations but should be discussed in public in advance")? It's both surprising to me and not consistent with what I knew. Filtering out sufficiently old versions is not news to me, at least to the extent it was filtering out things that actually might not work right (that it goes as rece
...
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3486413945)
@luke-jr Can you point out where your filtering of recent versions was discussed in public in advance of implementation (per the policy, e.g. "Behavior outside of these expectations may be reasonable in some situations but should be discussed in public in advance")? It's both surprising to me and not consistent with what I knew. Filtering out sufficiently old versions is not news to me, at least to the extent it was filtering out things that actually might not work right (that it goes as rece
...
💬 fanquake commented on pull request "depends: disable variables, rules and suffixes.":
(https://github.com/bitcoin/bitcoin/pull/33045#issuecomment-3486418579)
Guix build (x86_64):
```bash
e0edc03339b5b32705b8eacbc213a6fd6e5b787caba574671bb3c18340a141ec guix-build-52b1595850f6/output/aarch64-linux-gnu/SHA256SUMS.part
55778a17e64cabdb200679240fa004b5c5c9a684ecae773e8497997ab7c968d6 guix-build-52b1595850f6/output/aarch64-linux-gnu/bitcoin-52b1595850f6-aarch64-linux-gnu-debug.tar.gz
6a2f49dbc8238438d40d46c3ce699814ae1d9eb6d74dd0b4ff273db6648b181c guix-build-52b1595850f6/output/aarch64-linux-gnu/bitcoin-52b1595850f6-aarch64-linux-gnu.tar.gz
2fce927
...
(https://github.com/bitcoin/bitcoin/pull/33045#issuecomment-3486418579)
Guix build (x86_64):
```bash
e0edc03339b5b32705b8eacbc213a6fd6e5b787caba574671bb3c18340a141ec guix-build-52b1595850f6/output/aarch64-linux-gnu/SHA256SUMS.part
55778a17e64cabdb200679240fa004b5c5c9a684ecae773e8497997ab7c968d6 guix-build-52b1595850f6/output/aarch64-linux-gnu/bitcoin-52b1595850f6-aarch64-linux-gnu-debug.tar.gz
6a2f49dbc8238438d40d46c3ce699814ae1d9eb6d74dd0b4ff273db6648b181c guix-build-52b1595850f6/output/aarch64-linux-gnu/bitcoin-52b1595850f6-aarch64-linux-gnu.tar.gz
2fce927
...
👍 theStack approved a pull request: "crypto: Use secure_allocator for `AES256_ctx`"
(https://github.com/bitcoin/bitcoin/pull/31774#pullrequestreview-3416932085)
Code-review ACK 8bdcd12d3bcbeaf922fa10dc2a261848e3900cfd
(https://github.com/bitcoin/bitcoin/pull/31774#pullrequestreview-3416932085)
Code-review ACK 8bdcd12d3bcbeaf922fa10dc2a261848e3900cfd
🚀 fanquake merged a pull request: "depends: disable variables, rules and suffixes."
(https://github.com/bitcoin/bitcoin/pull/33045)
(https://github.com/bitcoin/bitcoin/pull/33045)
💬 laanwj commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2490867244)
Would prefer to generalize this function name to `DisableV1OnNetwork`. That it happens to be about clearnet is an implementation, not an interface detail.
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2490867244)
Would prefer to generalize this function name to `DisableV1OnNetwork`. That it happens to be about clearnet is an implementation, not an interface detail.
💬 brunoerg commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2490867264)
084bfbc1ec7f8f64f54d231bb641285622311b59: I think the CI failure is related to this `std::chrono::seconds::max()` (long long) because the `maximum_template_age` is used in `IntervalElapsed`as the interval which is a `uint32_t`.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2490867264)
084bfbc1ec7f8f64f54d231bb641285622311b59: I think the CI failure is related to this `std::chrono::seconds::max()` (long long) because the `maximum_template_age` is used in `IntervalElapsed`as the interval which is a `uint32_t`.
💬 l0rinc commented on pull request "random: scope environ extern to macOS, BSDs and Illumos":
(https://github.com/bitcoin/bitcoin/pull/33714#issuecomment-3486434684)
ACK 79d6f458e2300e1f47b94467cda233e1c761f8be
Thanks @hebasto for checking the other platforms.
(https://github.com/bitcoin/bitcoin/pull/33714#issuecomment-3486434684)
ACK 79d6f458e2300e1f47b94467cda233e1c761f8be
Thanks @hebasto for checking the other platforms.
💬 ismaelsadeeq commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486443571)
reACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 👾
There have been numerous changes to the header API since my last ACK.
I've reviewed the changes using the [compare diff](https://github.com/bitcoin/bitcoin/compare/a755e00a13541b3b5a707cf385f1cbec0449c6a9..6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969)
<details>
<summary>Changes</summary>
* Fixed typo in `Write`
* `LoggingConnection` now does not take Logging Options; a clear error message is returned or thrown when the logger connectio
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486443571)
reACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 👾
There have been numerous changes to the header API since my last ACK.
I've reviewed the changes using the [compare diff](https://github.com/bitcoin/bitcoin/compare/a755e00a13541b3b5a707cf385f1cbec0449c6a9..6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969)
<details>
<summary>Changes</summary>
* Fixed typo in `Write`
* `LoggingConnection` now does not take Logging Options; a clear error message is returned or thrown when the logger connectio
...
⚠️ Sjors opened an issue: "Mining interface tracking issue"
(https://github.com/bitcoin/bitcoin/issues/33777)
The mining interface is defined in:
- https://github.com/bitcoin/bitcoin/blob/master/src/interfaces/mining.h
- https://github.com/bitcoin/bitcoin/blob/master/src/ipc/capnp/mining.capnp
Clients that are known to depend on it:
- https://github.com/stratum-mining/sv2-tp
At this state it's OK to make breaking changes, such as deleting a method or changing its signature. But ideally we should do so once in a major release, collecting a bunch of fixes.
Non-breaking changes can be made any time, suc
...
(https://github.com/bitcoin/bitcoin/issues/33777)
The mining interface is defined in:
- https://github.com/bitcoin/bitcoin/blob/master/src/interfaces/mining.h
- https://github.com/bitcoin/bitcoin/blob/master/src/ipc/capnp/mining.capnp
Clients that are known to depend on it:
- https://github.com/stratum-mining/sv2-tp
At this state it's OK to make breaking changes, such as deleting a method or changing its signature. But ideally we should do so once in a major release, collecting a bunch of fixes.
Non-breaking changes can be made any time, suc
...
🚀 hebasto merged a pull request: "random: scope environ extern to macOS, BSDs and Illumos"
(https://github.com/bitcoin/bitcoin/pull/33714)
(https://github.com/bitcoin/bitcoin/pull/33714)
💬 fanquake commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486609787)
ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 - soon we'll be running bitcoin (kernel)
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486609787)
ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 - soon we'll be running bitcoin (kernel)
💬 theStack commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3486623443)
Concept ACK
Nit for the future: putting the move-only changes on the existing code (i.e. introduction of the `validate_snapshot_import` and `complete_background_validation` methods) into an own refactoring commit would make reviewing this a bit easier, I think.
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3486623443)
Concept ACK
Nit for the future: putting the move-only changes on the existing code (i.e. introduction of the `validate_snapshot_import` and `complete_background_validation` methods) into an own refactoring commit would make reviewing this a bit easier, I think.
🚀 fanquake merged a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595)
(https://github.com/bitcoin/bitcoin/pull/30595)
💬 l0rinc commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3486709813)
reACK a1df8547ca0711d99f05d1b31bc758a8068ba512
It's quite a heavy refactor, I did my best to review all the changes. Someone else should also review it to be sure.
The latest https://corecheck.dev/bitcoin/bitcoin/pulls/31713 Sonar warnings seem like false positives.
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3486709813)
reACK a1df8547ca0711d99f05d1b31bc758a8068ba512
It's quite a heavy refactor, I did my best to review all the changes. Someone else should also review it to be sure.
The latest https://corecheck.dev/bitcoin/bitcoin/pulls/31713 Sonar warnings seem like false positives.
💬 l0rinc commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3486739651)
Thanks @ryanofsky, given that this is in review for more than 1 year, I will address your concerns in a follow-up.
rfm?
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3486739651)
Thanks @ryanofsky, given that this is in review for more than 1 year, I will address your concerns in a follow-up.
rfm?
💬 ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3486744593)
Thanks for the review @ryanofsky and the coverage report @brunoerg
I will update the PR shortly with the suggestions and study the report.
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3486744593)
Thanks for the review @ryanofsky and the coverage report @brunoerg
I will update the PR shortly with the suggestions and study the report.
💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2491098502)
In 1a844b8ffd351d14b59de4cbae99b6e639068bb9 _bitcoin-mine: Extend example to mine a block_: just thought I got lucky on mainnet, but this does not distinguish between actually mining a block and a peer giving ups a new tip :-)
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2491098502)
In 1a844b8ffd351d14b59de4cbae99b6e639068bb9 _bitcoin-mine: Extend example to mine a block_: just thought I got lucky on mainnet, but this does not distinguish between actually mining a block and a peer giving ups a new tip :-)
💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3486757229)
re-ACK a2365f130c36f386caa8470528f39a34ac394798
Since my last review of e1f139b, commits 8bf2269efa and b094d38454 were absorbed into #33201 which added direct coverage of IPC via a functional test. This also allowed some simplification of `test/functional/interface_ipc_mining.py` which this PR adds.
I initially wanted to suggest folding this test into `interface_ipc.py`, but the latter has a dependency on PyCap.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3486757229)
re-ACK a2365f130c36f386caa8470528f39a34ac394798
Since my last review of e1f139b, commits 8bf2269efa and b094d38454 were absorbed into #33201 which added direct coverage of IPC via a functional test. This also allowed some simplification of `test/functional/interface_ipc_mining.py` which this PR adds.
I initially wanted to suggest folding this test into `interface_ipc.py`, but the latter has a dependency on PyCap.
💬 hebasto commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3486760754)
@151henry151
Sorry, but one more rebase is needed.
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3486760754)
@151henry151
Sorry, but one more rebase is needed.
💬 yuvicc commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486783430)
post-merge ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486783430)
post-merge ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969