✅ 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.
...
💬 MarcoFalke commented on pull request "ci: Move tidy to persistent worker":
(https://github.com/bitcoin/bitcoin/pull/28214#issuecomment-1671100821)
> It would be nice if we postpone moving macOS-cross job for a week or two.
Done for now. Should be trivial to re-ACK.
(https://github.com/bitcoin/bitcoin/pull/28214#issuecomment-1671100821)
> It would be nice if we postpone moving macOS-cross job for a week or two.
Done for now. Should be trivial to re-ACK.
👍 hebasto approved a pull request: "ci: Move tidy to persistent worker"
(https://github.com/bitcoin/bitcoin/pull/28214#pullrequestreview-1569282121)
re-ACK fa1d8955f69d3934f975f42eb04b5a3fc0d8aa35.
Thanks you.
(https://github.com/bitcoin/bitcoin/pull/28214#pullrequestreview-1569282121)
re-ACK fa1d8955f69d3934f975f42eb04b5a3fc0d8aa35.
Thanks you.
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288309113)
Enforcing/Documenting the lifetime behavior seems easier to understand with separate types (at least for me)
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288309113)
Enforcing/Documenting the lifetime behavior seems easier to understand with separate types (at least for me)
💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1288311782)
> Alternatively it could be disallowed properly.
converting from `uint256` can now only be done using `{Txid,Wtxid}::FromUint256`. This required less changes then expected so I think that is actually preferable.
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1288311782)
> Alternatively it could be disallowed properly.
converting from `uint256` can now only be done using `{Txid,Wtxid}::FromUint256`. This required less changes then expected so I think that is actually preferable.
📝 willcl-ark opened a pull request: "doc: Use GitHub's "Alert" markdown syntax"
(https://github.com/bitcoin/bitcoin/pull/28243)
GH has introduced new "Alert" syntax for md files (a.k.a "Admonitions" in other languages).
Whilst being wary of changing documentation just for the sake of "shiny" new features, in my opinion this is worthwhile in this case as we can use it to draw user attention to particularly important sections of docs.
The format is relatively backwards-compatible (i.e outside of GH), as the syntax is effectively a multi-block quotation, where the first line includes the alert type. This means the tex
...
(https://github.com/bitcoin/bitcoin/pull/28243)
GH has introduced new "Alert" syntax for md files (a.k.a "Admonitions" in other languages).
Whilst being wary of changing documentation just for the sake of "shiny" new features, in my opinion this is worthwhile in this case as we can use it to draw user attention to particularly important sections of docs.
The format is relatively backwards-compatible (i.e outside of GH), as the syntax is effectively a multi-block quotation, where the first line includes the alert type. This means the tex
...
💬 glozow commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#issuecomment-1671142331)
Fixed accidental drop of TrimToSize commit
(https://github.com/bitcoin/bitcoin/pull/25038#issuecomment-1671142331)
Fixed accidental drop of TrimToSize commit
📝 fanquake locked 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
...