📝 vincenzopalazzo opened a pull request: "Revert "Remove unused raw-pointer read helper from univalue""
(https://github.com/bitcoin/bitcoin/pull/28242)
This reverts commit fa940f41eaffa4b2a28c465a10a4c12d4b8976b8.
With this commit produce the following compilation error.
```
Making all in src
make[1]: Entering directory '/home/vincent/Github/bitcoin/src'
make[2]: Entering directory '/home/vincent/Github/bitcoin/src'
make[3]: Entering directory '/home/vincent/Github/bitcoin'
make[3]: Leaving directory '/home/vincent/Github/bitcoin'
GEN obj/build.h
CXX libbitcoin_util_a-clientversion.o
AR libbitcoin_util.a
C
...
(https://github.com/bitcoin/bitcoin/pull/28242)
This reverts commit fa940f41eaffa4b2a28c465a10a4c12d4b8976b8.
With this commit produce the following compilation error.
```
Making all in src
make[1]: Entering directory '/home/vincent/Github/bitcoin/src'
make[2]: Entering directory '/home/vincent/Github/bitcoin/src'
make[3]: Entering directory '/home/vincent/Github/bitcoin'
make[3]: Leaving directory '/home/vincent/Github/bitcoin'
GEN obj/build.h
CXX libbitcoin_util_a-clientversion.o
AR libbitcoin_util.a
C
...
💬 dergoegge commented on pull request "Revert "Remove unused raw-pointer read helper from univalue"":
(https://github.com/bitcoin/bitcoin/pull/28242#issuecomment-1671025800)
I had this too and iirc `make clean && make` worked for me.
(https://github.com/bitcoin/bitcoin/pull/28242#issuecomment-1671025800)
I had this too and iirc `make clean && make` worked for me.
💬 vincenzopalazzo commented on pull request "Revert "Remove unused raw-pointer read helper from univalue"":
(https://github.com/bitcoin/bitcoin/pull/28242#issuecomment-1671028960)
>I had this too and iirc make clean && make worked for me.
Mh, let me try
(https://github.com/bitcoin/bitcoin/pull/28242#issuecomment-1671028960)
>I had this too and iirc make clean && make worked for me.
Mh, let me try
💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1671034496)
Updated fe34609476b11cdd907c298f7300d424bf556e98 -> 9658d0dc17c270138523c41a982425e276b24271 ([pr28187.05](https://github.com/hebasto/bitcoin/commits/pr28187.05) -> [pr28187.06](https://github.com/hebasto/bitcoin/commits/pr28187.06), [diff](https://github.com/hebasto/bitcoin/compare/pr28187.05..pr28187.06)):
- addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1288122926)
- the workflow run name is set as the [commit message](https://docs.github.com/e
...
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1671034496)
Updated fe34609476b11cdd907c298f7300d424bf556e98 -> 9658d0dc17c270138523c41a982425e276b24271 ([pr28187.05](https://github.com/hebasto/bitcoin/commits/pr28187.05) -> [pr28187.06](https://github.com/hebasto/bitcoin/commits/pr28187.06), [diff](https://github.com/hebasto/bitcoin/compare/pr28187.05..pr28187.06)):
- addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1288122926)
- the workflow run name is set as the [commit message](https://docs.github.com/e
...
👍 fanquake approved a pull request: "ci: Use qemu-user through container engine"
(https://github.com/bitcoin/bitcoin/pull/28087#pullrequestreview-1569205742)
ACK fad0b67c212dcb8a16fcbda5a74acc959ed4e284 - this seems ok to me, and removes complexity from our CI system.
I've locally tested the ENVs changed here, and seen the associated slowdown / resource consumption. Had GCC segfault more than once when cross-compiling s390x.
(https://github.com/bitcoin/bitcoin/pull/28087#pullrequestreview-1569205742)
ACK fad0b67c212dcb8a16fcbda5a74acc959ed4e284 - this seems ok to me, and removes complexity from our CI system.
I've locally tested the ENVs changed here, and seen the associated slowdown / resource consumption. Had GCC segfault more than once when cross-compiling s390x.
🚀 fanquake merged a pull request: "ci: Use qemu-user through container engine"
(https://github.com/bitcoin/bitcoin/pull/28087)
(https://github.com/bitcoin/bitcoin/pull/28087)
💬 vincenzopalazzo commented on pull request "Revert "Remove unused raw-pointer read helper from univalue"":
(https://github.com/bitcoin/bitcoin/pull/28242#issuecomment-1671049248)
It worked, thanks! sorry for the confusion here
(https://github.com/bitcoin/bitcoin/pull/28242#issuecomment-1671049248)
It worked, thanks! sorry for the confusion here
✅ vincenzopalazzo closed a pull request: "Revert "Remove unused raw-pointer read helper from univalue""
(https://github.com/bitcoin/bitcoin/pull/28242)
(https://github.com/bitcoin/bitcoin/pull/28242)
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288266548)
> It is not possible to "move" a const reference into a shared_ptr. It will be a copy again.
Sorry, I messed up my git diff a bit, posting a slightly older version, `auto& ref = result->get_type();` has to be `auto&& ref = result->get_type();` (updated in the above diff now). With that, I think `std::move(ref)` is only called in case of an rvalue, and we use the (from what I understand without copy) `std::shared_ptr` constructor in case of an lvalue ref. I think that avoids unnecessary copie
...
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288266548)
> It is not possible to "move" a const reference into a shared_ptr. It will be a copy again.
Sorry, I messed up my git diff a bit, posting a slightly older version, `auto& ref = result->get_type();` has to be `auto&& ref = result->get_type();` (updated in the above diff now). With that, I think `std::move(ref)` is only called in case of an rvalue, and we use the (from what I understand without copy) `std::shared_ptr` constructor in case of an lvalue ref. I think that avoids unnecessary copie
...
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288269938)
> Sorry, I messed up my git diff a bit, posting a slightly older version, `auto& ref = result->get_type();` has to be `auto&& ref = result->get_type();` (updated in the above diff now). With that, I think `std::move(ref)` is only called in case of an rvalue, and we use the (from what I understand without copy) `std::shared_ptr` constructor in case of an lvalue ref. I think that avoids unnecessary copies?
Pretty sure it doesn't, when `get_type()` returns a `const std::string&`, both `auto&` an
...
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288269938)
> Sorry, I messed up my git diff a bit, posting a slightly older version, `auto& ref = result->get_type();` has to be `auto&& ref = result->get_type();` (updated in the above diff now). With that, I think `std::move(ref)` is only called in case of an rvalue, and we use the (from what I understand without copy) `std::shared_ptr` constructor in case of an lvalue ref. I think that avoids unnecessary copies?
Pretty sure it doesn't, when `get_type()` returns a `const std::string&`, both `auto&` an
...
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288270672)
> But if `get_type()` returns by value, how would we use a raw pointer? Who owns the underlying? That's why I went for `shared_ptr`, but I'm still easily confused by memory management so apologies if I'm being dim.
`std::optional` can be used to own optional memory, see my previous reply to the thread and the current version of the code :)
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288270672)
> But if `get_type()` returns by value, how would we use a raw pointer? Who owns the underlying? That's why I went for `shared_ptr`, but I'm still easily confused by memory management so apologies if I'm being dim.
`std::optional` can be used to own optional memory, see my previous reply to the thread and the current version of the code :)
👍 hebasto approved a pull request: "ci: Move tidy and macOS-cross to persistent workers"
(https://github.com/bitcoin/bitcoin/pull/28214#pullrequestreview-1569225408)
ACK fa960fefb589ac97de206f46d7b5c565fc70e3db. I have reviewed the code and it looks OK.
However, I'm a bit reluctant about running macOS-cross job on a persistent worker. I'd prefer run it on GitHub Actions due to the ability to create downloadable artifacts that is used in the QML repo to make designers' life a bit easier.
I understand, that there is no open PR with such a suggestion for now. But there is an ongoing Linux jobs related discussion in https://github.com/bitcoin-core/secp256k
...
(https://github.com/bitcoin/bitcoin/pull/28214#pullrequestreview-1569225408)
ACK fa960fefb589ac97de206f46d7b5c565fc70e3db. I have reviewed the code and it looks OK.
However, I'm a bit reluctant about running macOS-cross job on a persistent worker. I'd prefer run it on GitHub Actions due to the ability to create downloadable artifacts that is used in the QML repo to make designers' life a bit easier.
I understand, that there is no open PR with such a suggestion for now. But there is an ongoing Linux jobs related discussion in https://github.com/bitcoin-core/secp256k
...
💬 MarcoFalke commented on pull request "net: Use serialization parameters for CAddress serialization":
(https://github.com/bitcoin/bitcoin/pull/25284#discussion_r1288277494)
Good catch. The test was written by @vasild. I've removed the code, since there is already another unit test checking `SERIALIZE_METHODS_PARAMS`.
Also, I've replaced `DEC` by `RAW` to remove the dependency on tinyformat for the unit test and instead only use Bitcoin Core's own util functions.
(https://github.com/bitcoin/bitcoin/pull/25284#discussion_r1288277494)
Good catch. The test was written by @vasild. I've removed the code, since there is already another unit test checking `SERIALIZE_METHODS_PARAMS`.
Also, I've replaced `DEC` by `RAW` to remove the dependency on tinyformat for the unit test and instead only use Bitcoin Core's own util functions.
💬 MarcoFalke commented on pull request "net: Use serialization parameters for CAddress serialization":
(https://github.com/bitcoin/bitcoin/pull/25284#discussion_r1288277658)
Thanks, fixed doxygen
(https://github.com/bitcoin/bitcoin/pull/25284#discussion_r1288277658)
Thanks, fixed doxygen
💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1671071478)
> Had GCC segfault more than once when cross-compiling s390x.
I think this can be fixed by setting `MAKEJOBS="-j1" FILE_ENV=..`. (or a higher value, depending on your memory size)
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1671071478)
> Had GCC segfault more than once when cross-compiling s390x.
I think this can be fixed by setting `MAKEJOBS="-j1" FILE_ENV=..`. (or a higher value, depending on your memory size)
👋 MarcoFalke's pull request is ready for review: "ci: Use hard-coded root path for CI containers (bugfix)"
(https://github.com/bitcoin/bitcoin/pull/28185)
(https://github.com/bitcoin/bitcoin/pull/28185)
💬 MarcoFalke commented on pull request "ci: Use hard-coded root path for CI containers (bugfix)":
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1671075387)
Rebased on master to drop already merged bugfix commit d86a83d6b8587b0971e66c6910af23dd8c042969.
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1671075387)
Rebased on master to drop already merged bugfix commit d86a83d6b8587b0971e66c6910af23dd8c042969.
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288286773)
> Pretty sure it doesn't, when get_type() returns a const std::string&, both auto& and auto&& will just be a const std::string& as well.
Oh yes, I agree. So in that case, `ref` will be ` const std::string&` lvalue, and we'll use `std::shared_ptr<ret_type>(&ref, [](ret_type*) {});` where I don't think any copies happen?
> `std::optional` can be used to own optional memory
I know, I was answering your suggestion of using raw pointers instead of shared_ptr. `std::optional` solves the memor
...
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288286773)
> Pretty sure it doesn't, when get_type() returns a const std::string&, both auto& and auto&& will just be a const std::string& as well.
Oh yes, I agree. So in that case, `ref` will be ` const std::string&` lvalue, and we'll use `std::shared_ptr<ret_type>(&ref, [](ret_type*) {});` where I don't think any copies happen?
> `std::optional` can be used to own optional memory
I know, I was answering your suggestion of using raw pointers instead of shared_ptr. `std::optional` solves the memor
...
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288299967)
Ok, I see you are only using shared_ptr to wrap a raw pointer without a deleter.
Happy to switch to that, but I think it may be unintuitive, if sometimes a dev is allowed to keep the shared pointer around after the RPC, and sometimes not.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288299967)
Ok, I see you are only using shared_ptr to wrap a raw pointer without a deleter.
Happy to switch to that, but I think it may be unintuitive, if sometimes a dev is allowed to keep the shared pointer around after the RPC, and sometimes not.
💬 willcl-ark commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#issuecomment-1671097648)
Thanks for the conceptual review @dergoegge.
I've forced push a relatively different approach here, trying to take into consideration your review points, and #10738, but trying not to require introducing the decaying pointers. This has resulted in the following conceptual changes:
1. Taking the idea of @theuni's `GetnodesCopy()` in `m_nodes` iterations by re-purposing and upgrading `NodesSnapshot()` a bit, allowing removal of many instances of `m_nodes_mutex` in the code, as he achieved.
...
(https://github.com/bitcoin/bitcoin/pull/28222#issuecomment-1671097648)
Thanks for the conceptual review @dergoegge.
I've forced push a relatively different approach here, trying to take into consideration your review points, and #10738, but trying not to require introducing the decaying pointers. This has resulted in the following conceptual changes:
1. Taking the idea of @theuni's `GetnodesCopy()` in `m_nodes` iterations by re-purposing and upgrading `NodesSnapshot()` a bit, allowing removal of many instances of `m_nodes_mutex` in the code, as he achieved.
...