💬 hebasto commented on pull request "ci: Run "macOS 11.0 [gui, no tests] [jammy]" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293540862)
> Also, is the 10 GB enough to store all ccache + depends + image + ... stuff for all tasks?
Well, it depends on what "all tasks" encompass.
Anyway, ccache caches are to be updated on every run (that is not the case in libsecp repo, btw), which means that the current approach to handle Docker images is suboptimal (
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293540862)
> Also, is the 10 GB enough to store all ccache + depends + image + ... stuff for all tasks?
Well, it depends on what "all tasks" encompass.
Anyway, ccache caches are to be updated on every run (that is not the case in libsecp repo, btw), which means that the current approach to handle Docker images is suboptimal (
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1677428367)
Further steps require a bugfix: https://github.com/bitcoin/bitcoin/pull/28185
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1677428367)
Further steps require a bugfix: https://github.com/bitcoin/bitcoin/pull/28185
💬 dergoegge commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1293556312)
> Because, most of the time, the peer manager class will call to the CConMan setter and subsequently call the getter to utilize it.
I wasn't saying to make connman the container for this data, I only meant to add a setter to inform connman of the things it needs to perform its job (making connections to desirable nodes). The main container for the flags would still be peerman.
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1293556312)
> Because, most of the time, the peer manager class will call to the CConMan setter and subsequently call the getter to utilize it.
I wasn't saying to make connman the container for this data, I only meant to add a setter to inform connman of the things it needs to perform its job (making connections to desirable nodes). The main container for the flags would still be peerman.
💬 ryanofsky commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293558584)
> This sound like a bug in the source code, so I don't think it makes sense to document unreachable or unexpected code paths.
If it's a bug to to call the the Arg function with an unexpected type, it's important for the documentation to at say what types it is allowed to be called with, because I don't think there's another way of knowing this without a having a pretty deep understanding of the RPCHelpMan framework and digging into template and macro code.
If you just want the documentatio
...
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293558584)
> This sound like a bug in the source code, so I don't think it makes sense to document unreachable or unexpected code paths.
If it's a bug to to call the the Arg function with an unexpected type, it's important for the documentation to at say what types it is allowed to be called with, because I don't think there's another way of knowing this without a having a pretty deep understanding of the RPCHelpMan framework and digging into template and macro code.
If you just want the documentatio
...
👍 ryanofsky approved a pull request: "rpc: Add Arg() default helper"
(https://github.com/bitcoin/bitcoin/pull/28230#pullrequestreview-1577001175)
Approach ACK. I wouldn't say the API here is perfectly ideal because it doesn't check everything at compile time that could theoretically be checked at compile time. But it is a strict improvement that cuts verbosity and will only make other improvements easier in the future.
(https://github.com/bitcoin/bitcoin/pull/28230#pullrequestreview-1577001175)
Approach ACK. I wouldn't say the API here is perfectly ideal because it doesn't check everything at compile time that could theoretically be checked at compile time. But it is a strict improvement that cuts verbosity and will only make other improvements easier in the future.
💬 hebasto commented on pull request "ci: Use hard-coded root path for CI containers (bugfix)":
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1677454790)
> Currently the CI system will fail if the git folder that holds the Bitcoin Core source is moved from one location to another.
Are there any real-life use cases for this scenario?
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1677454790)
> Currently the CI system will fail if the git folder that holds the Bitcoin Core source is moved from one location to another.
Are there any real-life use cases for this scenario?
💬 dergoegge commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1293574085)
Sorry for being nitty about this but it's a little weird to have this set to some time that is different from the actual mock time of the node.
I would suggest initializing this to `None`, resetting it to `None` if `setmocktime(0)` was called and additionally assert that it is not None in `bumpmocktime`.
This ensures it is not set to some differing value (different from the actual mocktime) and makes sure `bumpmocktime` is used correctly. `TestNode.mocktime` can then also be reliably used
...
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1293574085)
Sorry for being nitty about this but it's a little weird to have this set to some time that is different from the actual mock time of the node.
I would suggest initializing this to `None`, resetting it to `None` if `setmocktime(0)` was called and additionally assert that it is not None in `bumpmocktime`.
This ensures it is not set to some differing value (different from the actual mocktime) and makes sure `bumpmocktime` is used correctly. `TestNode.mocktime` can then also be reliably used
...
💬 MarcoFalke commented on pull request "ci: Use hard-coded root path for CI containers (bugfix)":
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1677470954)
Yes, Cirrus CI persistent workers, and potentially GitHub (or other's) persistent workers (haven't checked).
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1677470954)
Yes, Cirrus CI persistent workers, and potentially GitHub (or other's) persistent workers (haven't checked).
💬 hebasto commented on pull request "ci: Use hard-coded root path for CI containers (bugfix)":
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1677477117)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1677477117)
Concept ACK.
💬 MarcoFalke commented on pull request "ci: Use hard-coded root path for CI containers (bugfix)":
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1677484817)
Also, a developer may be moving the git folder at any time.
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1677484817)
Also, a developer may be moving the git folder at any time.
💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1293586086)
Done. I like that the assertion forces tests to make an initial setmocktime call before calling bumpmocktime.
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1293586086)
Done. I like that the assertion forces tests to make an initial setmocktime call before calling bumpmocktime.
✅ fanquake closed an issue: "wallet_importdescriptors.py: can't decode bytes in position 228861-228863: unexpected end of data"
(https://github.com/bitcoin/bitcoin/issues/28030)
(https://github.com/bitcoin/bitcoin/issues/28030)
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293608139)
> it's important for the documentation to at say what types it is allowed to be called with
I guess long term it could make sense to make `RPCArg::Type` an enum of C++ types and then have the documentation say that the type passed to `Arg<Type>(i)` must match exactly the corresponding `RPCArg::Type`.
For now, my recommendation would be to call `Arg<std::string>(i)` for `STR*` args, `Arg<bool>(i)` for `BOOL` args, and `Arg<double/(u)int${n}_t>(i)` for `NUM` args.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293608139)
> it's important for the documentation to at say what types it is allowed to be called with
I guess long term it could make sense to make `RPCArg::Type` an enum of C++ types and then have the documentation say that the type passed to `Arg<Type>(i)` must match exactly the corresponding `RPCArg::Type`.
For now, my recommendation would be to call `Arg<std::string>(i)` for `STR*` args, `Arg<bool>(i)` for `BOOL` args, and `Arg<double/(u)int${n}_t>(i)` for `NUM` args.
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293610456)
Would it be fine to add "The `Type` passed to this helper must match the corresponding `RPCArg::Type`" to the doxygen of `Arg`?
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293610456)
Would it be fine to add "The `Type` passed to this helper must match the corresponding `RPCArg::Type`" to the doxygen of `Arg`?
🚀 fanquake merged a pull request: "test: locked_wallet, skip default fee estimation"
(https://github.com/bitcoin/bitcoin/pull/28232)
(https://github.com/bitcoin/bitcoin/pull/28232)
💬 MarcoFalke commented on pull request "ci: Run "macOS 11.0 [gui, no tests] [jammy]" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293618511)
> Artifacts are subjects of 90 days [retention period](https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts). Therefore, I still lean to ghcr.io.
A re-build every 3 months seems perfectly fine. ghcr is *currently* free and unlimited, but I can't imagine this being the case forever. (The whole reason this pull was opened is because we relied on a service assuming it will be free and unlimited forever). Also, ghcr requires write access.
In any case, CI images
...
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293618511)
> Artifacts are subjects of 90 days [retention period](https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts). Therefore, I still lean to ghcr.io.
A re-build every 3 months seems perfectly fine. ghcr is *currently* free and unlimited, but I can't imagine this being the case forever. (The whole reason this pull was opened is because we relied on a service assuming it will be free and unlimited forever). Also, ghcr requires write access.
In any case, CI images
...
💬 iBeizsley commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1677542038)
> Secondly, on the basis of mempool consistency with miners, since full-rbf is the compatible policy in almost all cases, we should be supporting it by default if you take the position that we want to optimize for consistency with miners. (CC: @achow101)
If 37% of hash rate is mining full RBF, enabling full RBF is (currently) a move away from consistency with majority hash rate.
> Thirdly, all previous arguments in favor of full-rbf, such as multiparty transactions, wallets, user-expectations,
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1677542038)
> Secondly, on the basis of mempool consistency with miners, since full-rbf is the compatible policy in almost all cases, we should be supporting it by default if you take the position that we want to optimize for consistency with miners. (CC: @achow101)
If 37% of hash rate is mining full RBF, enabling full RBF is (currently) a move away from consistency with majority hash rate.
> Thirdly, all previous arguments in favor of full-rbf, such as multiparty transactions, wallets, user-expectations,
...
💬 hebasto commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#issuecomment-1677548617)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27793#issuecomment-1677548617)
Concept ACK.
👍 dergoegge approved a pull request: "test: tx orphan handling"
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1577100942)
reACK 9eac5a0529f869075f0331e40d322c34fc8fc2af
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1577100942)
reACK 9eac5a0529f869075f0331e40d322c34fc8fc2af
💬 MarcoFalke commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#discussion_r1293632896)
> Yeah, would you prefer it like this?
No preference. Currently it is wrong, so you'll have to remove it, or fix it, or close this pull.
(https://github.com/bitcoin/bitcoin/pull/27793#discussion_r1293632896)
> Yeah, would you prefer it like this?
No preference. Currently it is wrong, so you'll have to remove it, or fix it, or close this pull.