💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782094771)
Sounds fair to me - made this change 👍
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782094771)
Sounds fair to me - made this change 👍
💬 maflcko commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782120451)
This is a breaking change and will cause the tests to fail, not a documentation change
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782120451)
This is a breaking change and will cause the tests to fail, not a documentation change
💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2384840093)
> the next step should be revert #26564?.
IIUC the motivation for 26564 was to have a fully static and fixed path for the unit test datadir. My understanding is that changing the temp storage device was just a side-effect and not the primary motivation. (My comment was just a note to say that your goal is already achievable today)
> It seems more natural to plug the already existing -testdatadir arg to the benchmark framework.
I agree. I've also suggested this in https://github.com/bitc
...
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2384840093)
> the next step should be revert #26564?.
IIUC the motivation for 26564 was to have a fully static and fixed path for the unit test datadir. My understanding is that changing the temp storage device was just a side-effect and not the primary motivation. (My comment was just a note to say that your goal is already achievable today)
> It seems more natural to plug the already existing -testdatadir arg to the benchmark framework.
I agree. I've also suggested this in https://github.com/bitc
...
💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1782146967)
```suggestion
| [Qt](../depends/packages/qt.mk) | [link](https://download.qt.io/official_releases/qt/) | [6.7.2](https://github.com/bitcoin/bitcoin/pull/30997) | [6.2](https://github.com/bitcoin/bitcoin/pull/30997) | No |
```
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1782146967)
```suggestion
| [Qt](../depends/packages/qt.mk) | [link](https://download.qt.io/official_releases/qt/) | [6.7.2](https://github.com/bitcoin/bitcoin/pull/30997) | [6.2](https://github.com/bitcoin/bitcoin/pull/30997) | No |
```
💬 Mackain commented on pull request "doc: update IBD requirements in doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2384959633)
> ACK [3208df2](https://github.com/bitcoin/bitcoin/commit/3208df2a100f58569165081cd20c02abed827286)
>
> Might be good to update the PR description to reflect the changes (and the commit message as well, if you need to repush).
>
> Suggestion for the PR title: `doc: update IBD requirements in doc/README.md`
@jonatack thanks! Updated the PR title as you suggested. The commit message was created automatically when I clicked the "Commit Suggestion" button on your previous feedback. Fixing i
...
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2384959633)
> ACK [3208df2](https://github.com/bitcoin/bitcoin/commit/3208df2a100f58569165081cd20c02abed827286)
>
> Might be good to update the PR description to reflect the changes (and the commit message as well, if you need to repush).
>
> Suggestion for the PR title: `doc: update IBD requirements in doc/README.md`
@jonatack thanks! Updated the PR title as you suggested. The commit message was created automatically when I clicked the "Commit Suggestion" button on your previous feedback. Fixing i
...
💬 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