💬 Sjors commented on issue "Add IPv6 pinhole support using UPnP / NAT-PMP":
(https://github.com/bitcoin/bitcoin/issues/17012#issuecomment-2384986010)
@ffrediani it might make sense to turn this on by default in the future. But first we should wait a while to make sure there's no bugs in #30043. Afaik there's currently no shortage of listening nodes, but that could change over time.
(https://github.com/bitcoin/bitcoin/issues/17012#issuecomment-2384986010)
@ffrediani it might make sense to turn this on by default in the future. But first we should wait a while to make sure there's no bugs in #30043. Afaik there's currently no shortage of listening nodes, but that could change over time.
💬 maflcko commented on pull request "doc: update IBD requirements in doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2384987847)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2384987847)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 Sjors commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1782244032)
Ok, well the numbering here doesn't conflict with #30955 and there's no other proposed interface change pending. So I'll keep the renumbering here.
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1782244032)
Ok, well the numbering here doesn't conflict with #30955 and there's no other proposed interface change pending. So I'll keep the renumbering here.
💬 maflcko commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2384997720)
Only change is https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1781022698
re-ACK 4f33e9a85c3a8f546deddc2f78cf74bceff30315 👾
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N9
...
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2384997720)
Only change is https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1781022698
re-ACK 4f33e9a85c3a8f546deddc2f78cf74bceff30315 👾
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N9
...
💬 Mackain commented on pull request "doc: update IBD requirements in doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2385016043)
> Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
@maflcko thanks for pointing that out, fixing now
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2385016043)
> Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
@maflcko thanks for pointing that out, fixing now
💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2385041236)
Rebased after #30043.
> TSAN compile failure fixed in [chaincodelabs/libmultiprocess#113](https://github.com/chaincodelabs/libmultiprocess/pull/113).
Added a commit to bump libmultiprocess to include this (can be its own PR later).
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2385041236)
Rebased after #30043.
> TSAN compile failure fixed in [chaincodelabs/libmultiprocess#113](https://github.com/chaincodelabs/libmultiprocess/pull/113).
Added a commit to bump libmultiprocess to include this (can be its own PR later).
🤔 maflcko reviewed a pull request: "Cover remaining tinyformat usages in CheckFormatSpecifiers"
(https://github.com/bitcoin/bitcoin/pull/30999#pullrequestreview-2339348510)
As mentioned previously, it looks like there is one correct commit. However, I have a hard time seeing how the others are useful in a great picture, given that some of them are incomplete anyway.
(https://github.com/bitcoin/bitcoin/pull/30999#pullrequestreview-2339348510)
As mentioned previously, it looks like there is one correct commit. However, I have a hard time seeing how the others are useful in a great picture, given that some of them are incomplete anyway.
💬 maflcko commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1782271399)
not sure about implementing a random and specific subset of `*` in specifiers. I think it is easier to either fully support them, or not at all. But having developers read the parser to understand which subset they are allowed to use may be causing more frustration than solving any real issue.
<!--
format("%-*.*f", 9, 2, 2.13)
https://godbolt.org/z/YoqfxEjMz
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1782271399)
not sure about implementing a random and specific subset of `*` in specifiers. I think it is easier to either fully support them, or not at all. But having developers read the parser to understand which subset they are allowed to use may be causing more frustration than solving any real issue.
<!--
format("%-*.*f", 9, 2, 2.13)
https://godbolt.org/z/YoqfxEjMz
💬 maflcko commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1782276063)
Not sure about moving this out. This will break the doxygen comment above. Also, it drops the "detail-namespace".
Generally, I think that test-only code should follow the real code, not the other way round. As long as real code is testable, optimizing other parts of the unit tests doesn't seem too useful, especially if it breaks the existing construct and documentation.
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1782276063)
Not sure about moving this out. This will break the doxygen comment above. Also, it drops the "detail-namespace".
Generally, I think that test-only code should follow the real code, not the other way round. As long as real code is testable, optimizing other parts of the unit tests doesn't seem too useful, especially if it breaks the existing construct and documentation.
💬 maflcko commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1782259660)
> Compared to `PassFmt` I found the `ValidFormatSpecifiers` to be more specific (I'm not a fan of abbrvs and // comments).
I don't care about naming, so if you want to rename `PassFmt` to something else, this is fine. However, the `//` comment isn't useless: It explains that the goal of this helper function is to be close as possible to the real code (and document the only difference). I found that useful as a single compilation unit that serves as a close proxy to the real code, with almost
...
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1782259660)
> Compared to `PassFmt` I found the `ValidFormatSpecifiers` to be more specific (I'm not a fan of abbrvs and // comments).
I don't care about naming, so if you want to rename `PassFmt` to something else, this is fine. However, the `//` comment isn't useless: It explains that the goal of this helper function is to be close as possible to the real code (and document the only difference). I found that useful as a single compilation unit that serves as a close proxy to the real code, with almost
...
💬 darosior commented on pull request "refactor: Appropriate re-naming of MAX_OPCODE after tapscript":
(https://github.com/bitcoin/bitcoin/pull/30953#issuecomment-2385048924)
Concept NACK. Not that this change makes things worse, but i don't think it meets [the bar for refactorings](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring).
(https://github.com/bitcoin/bitcoin/pull/30953#issuecomment-2385048924)
Concept NACK. Not that this change makes things worse, but i don't think it meets [the bar for refactorings](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring).
💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2385057317)
Depends builds on macOS now, but the next step fails:
```
cmake -B build-depends --toolchain depends/x86_64-apple-darwin24.0.0/toolchain.cmake
...
CMake Error at /usr/local/Cellar/cmake/3.29.2/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
Could NOT find Qt (missing: Qt6_FOUND) (found suitable version "6.7.2",
minimum required is "6.2")
Call Stack (most recent call first):
/usr/local/Cellar/cmake/3.29.2/share/cmake/Modules/FindPackageHandleStandardArgs.cma
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2385057317)
Depends builds on macOS now, but the next step fails:
```
cmake -B build-depends --toolchain depends/x86_64-apple-darwin24.0.0/toolchain.cmake
...
CMake Error at /usr/local/Cellar/cmake/3.29.2/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
Could NOT find Qt (missing: Qt6_FOUND) (found suitable version "6.7.2",
minimum required is "6.2")
Call Stack (most recent call first):
/usr/local/Cellar/cmake/3.29.2/share/cmake/Modules/FindPackageHandleStandardArgs.cma
...
💬 l0rinc commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1782312001)
> not implemented to minimize code complexity as long as they are not used in the codebase
I've implemented the part that was used, not a "random subset"
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1782312001)
> not implemented to minimize code complexity as long as they are not used in the codebase
I've implemented the part that was used, not a "random subset"
✅ l0rinc closed a pull request: "Cover remaining tinyformat usages in CheckFormatSpecifiers"
(https://github.com/bitcoin/bitcoin/pull/30999)
(https://github.com/bitcoin/bitcoin/pull/30999)
💬 l0rinc commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#issuecomment-2385096310)
I'm closing it for lack of interest, feel free to cherry-pick changes to other PRs
(https://github.com/bitcoin/bitcoin/pull/30999#issuecomment-2385096310)
I'm closing it for lack of interest, feel free to cherry-pick changes to other PRs
💬 maflcko commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2385099228)
Have you seen https://github.com/bitcoin/bitcoin/pull/30592/files#r1710942068 ?
I am now thinking this should probably be done in this pull request. Otherwise there will be another follow-up for a trivial doc update.
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2385099228)
Have you seen https://github.com/bitcoin/bitcoin/pull/30592/files#r1710942068 ?
I am now thinking this should probably be done in this pull request. Otherwise there will be another follow-up for a trivial doc update.
📝 maflcko reopened a pull request: "Fix unsigned integer overflows in interpreter"
(https://github.com/bitcoin/bitcoin/pull/24214)
Unsigned integer overflow is well defined by the language and in some cases even useful or necessary. However, I think that it should be avoided in interpreter, as it makes the code harder to read and requires the whole file to be suppressed in the sanitizer. This puts more burden on reviewers to check that any changes to interpreter that involve unsigned integer overflow are sane.
This patch involves a few changes:
* Evaluate the addition in 64-bit "space". Previously, the first argument wa
...
(https://github.com/bitcoin/bitcoin/pull/24214)
Unsigned integer overflow is well defined by the language and in some cases even useful or necessary. However, I think that it should be avoided in interpreter, as it makes the code harder to read and requires the whole file to be suppressed in the sanitizer. This puts more burden on reviewers to check that any changes to interpreter that involve unsigned integer overflow are sane.
This patch involves a few changes:
* Evaluate the addition in 64-bit "space". Previously, the first argument wa
...
💬 willcl-ark commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#issuecomment-2385169958)
Concept ACK.
I wonder if it's not easier to minimize what is being caught in the IOError try block and just remove missing bins from the `BINARIES` list? This list is used later on in the script too so in my opinion that is probably the better option. Otherwise we will end up referencing non-existing binaries/manpages in the existing manpages. Something like:
```diff
diff --git a/contrib/devtools/gen-manpages.py b/contrib/devtools/gen-manpages.py
index 92acd9a4037..0e0d31807bd 100755
--
...
(https://github.com/bitcoin/bitcoin/pull/30986#issuecomment-2385169958)
Concept ACK.
I wonder if it's not easier to minimize what is being caught in the IOError try block and just remove missing bins from the `BINARIES` list? This list is used later on in the script too so in my opinion that is probably the better option. Otherwise we will end up referencing non-existing binaries/manpages in the existing manpages. Something like:
```diff
diff --git a/contrib/devtools/gen-manpages.py b/contrib/devtools/gen-manpages.py
index 92acd9a4037..0e0d31807bd 100755
--
...
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1782379621)
I can't reach this condition. Even when I spin up a mainnet node and immediately start `bitcoin-mine` it just seems to wait a bit and then return a hash.
Maybe we should simplify the `getTip()` interface to not return an optional. Though I'm not sure if it's completely impossible to hit this.
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1782379621)
I can't reach this condition. Even when I spin up a mainnet node and immediately start `bitcoin-mine` it just seems to wait a bit and then return a hash.
Maybe we should simplify the `getTip()` interface to not return an optional. Though I'm not sure if it's completely impossible to hit this.
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2385186175)
tACK 2227afac0372358287fb879f3b0bd07fd653f3f8
Except I noticed that `bitcoin-mine -debug=ipc` doesn't print anything, which is surprising.
Tested the standalone application. I plan to do another rebase of https://github.com/Sjors/bitcoin/pull/48 on top of this later (which turns `bitcoin-mine` into a Template Provider).
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2385186175)
tACK 2227afac0372358287fb879f3b0bd07fd653f3f8
Except I noticed that `bitcoin-mine -debug=ipc` doesn't print anything, which is surprising.
Tested the standalone application. I plan to do another rebase of https://github.com/Sjors/bitcoin/pull/48 on top of this later (which turns `bitcoin-mine` into a Template Provider).