💬 fanquake commented on pull request "build: Pointer Authentication and Branch Target Identification for aarch64 Linux (Guix)":
(https://github.com/bitcoin/bitcoin/pull/24123#discussion_r1293395019)
Made this Guix-only for now.
(https://github.com/bitcoin/bitcoin/pull/24123#discussion_r1293395019)
Made this Guix-only for now.
💬 fanquake commented on pull request "build: LLVM 16 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1677230335)
Re-rebased on #27897, and updated 6f6552688233e35a56f7a4c428c99c657150d313 to include `lld-15` & `lld-as-ld-wrapper-15`. Using lld-as-ld-wrapper seems to be the best way to make build systems (like Qts) work with LLD.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1677230335)
Re-rebased on #27897, and updated 6f6552688233e35a56f7a4c428c99c657150d313 to include `lld-15` & `lld-as-ld-wrapper-15`. Using lld-as-ld-wrapper seems to be the best way to make build systems (like Qts) work with LLD.
💬 fanquake commented on pull request "guix: Use LTO to build releases":
(https://github.com/bitcoin/bitcoin/pull/25391#issuecomment-1677251367)
Rebased on #27897. Simplifed / squashed down the changes here. Updated the PR description. The current (non-conceptual) blocker is that Qt for Windows doesn't really work under LTO... See 8031e400af66bf5267201069a7c062cb6a9d02ea.
(https://github.com/bitcoin/bitcoin/pull/25391#issuecomment-1677251367)
Rebased on #27897. Simplifed / squashed down the changes here. Updated the PR description. The current (non-conceptual) blocker is that Qt for Windows doesn't really work under LTO... See 8031e400af66bf5267201069a7c062cb6a9d02ea.
💬 dergoegge commented on issue "meta: Isolated fuzzing of net processing":
(https://github.com/bitcoin/bitcoin/issues/27502#issuecomment-1677257433)
> What is this going to fuzz exactly? Is this intended to be abstracted away from validation/mempool behaviours (ie, stubbing them out)?
Mostly all code in `net_processing.cpp` (and relevant modules like orphanage, tx tracker) but excluding net, validation or mempool logic. Stubbing the latter out gives us control over testing net processing behavior related to these adjacent modules by being able to mock their behavior and state.
I'm not saying that we shouldn't fuzz the other modules, ju
...
(https://github.com/bitcoin/bitcoin/issues/27502#issuecomment-1677257433)
> What is this going to fuzz exactly? Is this intended to be abstracted away from validation/mempool behaviours (ie, stubbing them out)?
Mostly all code in `net_processing.cpp` (and relevant modules like orphanage, tx tracker) but excluding net, validation or mempool logic. Stubbing the latter out gives us control over testing net processing behavior related to these adjacent modules by being able to mock their behavior and state.
I'm not saying that we shouldn't fuzz the other modules, ju
...
💬 dergoegge commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1293423583)
... and set self.mocktime when ever setmocktime is called?
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1293423583)
... and set self.mocktime when ever setmocktime is called?
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293454810)
> > Also, the value will be nullptr before and after the call to HandleRequest
>
> Does this also hold if `m_fun` throws an exception?
The object does not exist after an exception. It will be destroyed/deallocated for each RPC call, see https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285851447
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293454810)
> > Also, the value will be nullptr before and after the call to HandleRequest
>
> Does this also hold if `m_fun` throws an exception?
The object does not exist after an exception. It will be destroyed/deallocated for each RPC call, see https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285851447
💬 dergoegge commented on pull request "[no merge, meta] refactor: net/net processing split":
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1677329965)
> Being immediately dismissed out of hand feels pretty frustrating.
I apologize, that wasn't my intention.
I want to make it clear though that this PR is about continuing previous decoupling work that has seemingly dropped of peoples radar when previous contributors switched focus or left the project. I do not think the previous work went in the wrong direction nor that this breaks potentially multi threading the message processing code in the future.
If you think this design is somehow
...
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1677329965)
> Being immediately dismissed out of hand feels pretty frustrating.
I apologize, that wasn't my intention.
I want to make it clear though that this PR is about continuing previous decoupling work that has seemingly dropped of peoples radar when previous contributors switched focus or left the project. I do not think the previous work went in the wrong direction nor that this breaks potentially multi threading the message processing code in the future.
If you think this design is somehow
...
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293469146)
I've made `bool no_default` a function `CheckNoDefault`, because this is exactly what needs to be checked. Let me know if you still disagree.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293469146)
I've made `bool no_default` a function `CheckNoDefault`, because this is exactly what needs to be checked. Let me know if you still disagree.
💬 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.