💬 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).
💬 willcl-ark commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2385221622)
I very much like the idea of the wrapper approach described in https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-237920497, it feels to me like the correct way to structure and package an application consisting of multiple binaries rather than overloading existing binaries with even more options.
My only reservation with the approach is the potential magnitude of the effects downstream. If executed poorly this could break many scripts, programs and general infra. However, so long a
...
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2385221622)
I very much like the idea of the wrapper approach described in https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-237920497, it feels to me like the correct way to structure and package an application consisting of multiple binaries rather than overloading existing binaries with even more options.
My only reservation with the approach is the potential magnitude of the effects downstream. If executed poorly this could break many scripts, programs and general infra. However, so long a
...
💬 itornaza commented on pull request "refactor: Appropriate re-naming of MAX_OPCODE after tapscript":
(https://github.com/bitcoin/bitcoin/pull/30953#issuecomment-2385222900)
> 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).
Thank you for the clarification @darosior, and I will make sure to aim higher in the future to match the standards. However, the name of this variable after the introduction of tapscript is misleading and especially to new contributors like myself that read through the code and try to understand it. With the
...
(https://github.com/bitcoin/bitcoin/pull/30953#issuecomment-2385222900)
> 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).
Thank you for the clarification @darosior, and I will make sure to aim higher in the future to match the standards. However, the name of this variable after the introduction of tapscript is misleading and especially to new contributors like myself that read through the code and try to understand it. With the
...
💬 maflcko commented on pull request "refactor: Avoid unsigned integer overflow in `script/interpreter.cpp`":
(https://github.com/bitcoin/bitcoin/pull/29543#issuecomment-2385290012)
Re-opened the other one
(https://github.com/bitcoin/bitcoin/pull/29543#issuecomment-2385290012)
Re-opened the other one
💬 maflcko commented on pull request "log: Enforce trailing newline":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1782454152)
Sure, done
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1782454152)
Sure, done
💬 maflcko commented on pull request "log: Enforce trailing newline":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1782454398)
Sure, done both
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1782454398)
Sure, done both
💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#issuecomment-2385303342)
(rebased)
(https://github.com/bitcoin/bitcoin/pull/30967#issuecomment-2385303342)
(rebased)
💬 maflcko commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-2385328826)
Are you still working on this? 28.x will be released without this, so it would be good to explain why it should be merged for 29.x (or later), possibly in a release note. I haven't looked if there is any unaddressed feedback waiting for your reply, but at a minimum you'll have to rebase this for a fresh CI run and to pick up the other AU stuff.
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-2385328826)
Are you still working on this? 28.x will be released without this, so it would be good to explain why it should be merged for 29.x (or later), possibly in a release note. I haven't looked if there is any unaddressed feedback waiting for your reply, but at a minimum you'll have to rebase this for a fresh CI run and to pick up the other AU stuff.
💬 maflcko commented on issue "intermittent issue in wallet_upgradewallet.py AssertionError: [node 2] Node returned unexpected exit code (1) vs (0) when stopping":
(https://github.com/bitcoin/bitcoin/issues/30798#issuecomment-2385338965)
https://cirrus-ci.com/task/4769312399949824?logs=ci#L3305
(https://github.com/bitcoin/bitcoin/issues/30798#issuecomment-2385338965)
https://cirrus-ci.com/task/4769312399949824?logs=ci#L3305