💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293469639)
Added more checks. (`CheckRequiredOrDefault`)
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293469639)
Added more checks. (`CheckRequiredOrDefault`)
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293478254)
Left this as-is in the header, because the implementation already documents this :sweat_smile:
However, I've added your docs to the new `Check*` functions.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293478254)
Left this as-is in the header, because the implementation already documents this :sweat_smile:
However, I've added your docs to the new `Check*` functions.
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293478970)
Sure, mind providing a patch that compiles, which I can take?
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293478970)
Sure, mind providing a patch that compiles, which I can take?
💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1293480751)
Added a wrapper for the RPC to update `TestNode::mocktime` every time `setmocktime` is called.
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1293480751)
Added a wrapper for the RPC to update `TestNode::mocktime` every time `setmocktime` is called.
💬 vasild commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1293481061)
Good catch with this deficiency!
If `m_added_nodes` is changed to `std::unordered_set`, then this can be a simple `m_added_nodes.count() > 0` and `CConnman::AddNode()` and `CConnman::RemoveAddedNode()` could be simplified (and made faster since they are now `O(number of added nodes)` and will become `O(1)`, I guess performance is irrelevant in practice because of the small number of elements).
Maybe this is worth a separate PR since it is distinct from other commits (one commit to change i
...
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1293481061)
Good catch with this deficiency!
If `m_added_nodes` is changed to `std::unordered_set`, then this can be a simple `m_added_nodes.count() > 0` and `CConnman::AddNode()` and `CConnman::RemoveAddedNode()` could be simplified (and made faster since they are now `O(number of added nodes)` and will become `O(1)`, I guess performance is irrelevant in practice because of the small number of elements).
Maybe this is worth a separate PR since it is distinct from other commits (one commit to change i
...
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293509108)
> nit/bikeshed: I'd prefer `Arg` over `ArgOrDefault`.
Agree. Also used `MaybeArg` over `ArgOrNot`.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293509108)
> nit/bikeshed: I'd prefer `Arg` over `ArgOrDefault`.
Agree. Also used `MaybeArg` over `ArgOrNot`.
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#issuecomment-1677389241)
Sorry for the repeated force-pushes, but I think I've addressed/replied to all feedback for now.
(https://github.com/bitcoin/bitcoin/pull/28230#issuecomment-1677389241)
Sorry for the repeated force-pushes, but I think I've addressed/replied to all feedback for now.
💬 fanquake commented on pull request "build: prune Boost headers in depends":
(https://github.com/bitcoin/bitcoin/pull/24742#issuecomment-1677390124)
Closing for now.
(https://github.com/bitcoin/bitcoin/pull/24742#issuecomment-1677390124)
Closing for now.
✅ fanquake closed a pull request: "build: prune Boost headers in depends"
(https://github.com/bitcoin/bitcoin/pull/24742)
(https://github.com/bitcoin/bitcoin/pull/24742)
💬 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_r1293524063)
> Yes, it will happen from time to time. But not every run :)
Assuming a total of 10 GitHub tasks with 500 MB of caches, it will happen every second run. Either way, I am still wondering:
> Also, is the 10 GB enough to store all ccache + depends + image + ... stuff for all tasks?
> There is an alternative to that approach, which is using Container Registry for images (with write package permission).
What about artefacts? (Ref. discussion in the https://github.com/bitcoin-core/secp256
...
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293524063)
> Yes, it will happen from time to time. But not every run :)
Assuming a total of 10 GitHub tasks with 500 MB of caches, it will happen every second run. Either way, I am still wondering:
> Also, is the 10 GB enough to store all ccache + depends + image + ... stuff for all tasks?
> There is an alternative to that approach, which is using Container Registry for images (with write package permission).
What about artefacts? (Ref. discussion in the https://github.com/bitcoin-core/secp256
...
💬 ryanofsky commented on pull request "refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate":
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1293531803)
> Are there any plans to remove the chainman reference from `Chainstate`? I really don't like this pattern in general, i.e. managed object having a reference to the manager. Imo, it is an indication that the responsibilities of `Chainstate` and `ChainstateManager` are not well defined.
I agree, and if I were cleaning this up would get rid of this back reference and make a lot of ChainState members functions into standalone functions that take explicit ChainState and ChainStateManager argument
...
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1293531803)
> Are there any plans to remove the chainman reference from `Chainstate`? I really don't like this pattern in general, i.e. managed object having a reference to the manager. Imo, it is an indication that the responsibilities of `Chainstate` and `ChainstateManager` are not well defined.
I agree, and if I were cleaning this up would get rid of this back reference and make a lot of ChainState members functions into standalone functions that take explicit ChainState and ChainStateManager argument
...
💬 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_r1293535111)
> What about artefacts? (Ref. discussion in the [bitcoin-core/secp256k1#1398 (comment)](https://github.com/bitcoin-core/secp256k1/pull/1398#issuecomment-1677074743))
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 ghct.io.
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293535111)
> What about artefacts? (Ref. discussion in the [bitcoin-core/secp256k1#1398 (comment)](https://github.com/bitcoin-core/secp256k1/pull/1398#issuecomment-1677074743))
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 ghct.io.
💬 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).