👋 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
...
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288338213)
That's a good point. To confirm my understanding: the lifecycles are identical for both the (1) hybrid std::optional/pointer and the (2) uniform pointer approach (i.e. in both cases, reference types can only be used during the RPC call, value types can be used whenever), but for (2) the developer (user) would have to dive into the implementation to figure out what the exact lifecycle is, whereas if we use different types it's obvious from just looking at the return type.
I'm going to think ab
...
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288338213)
That's a good point. To confirm my understanding: the lifecycles are identical for both the (1) hybrid std::optional/pointer and the (2) uniform pointer approach (i.e. in both cases, reference types can only be used during the RPC call, value types can be used whenever), but for (2) the developer (user) would have to dive into the implementation to figure out what the exact lifecycle is, whereas if we use different types it's obvious from just looking at the return type.
I'm going to think ab
...
💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1671159616)
re-ACK 9658d0dc17c270138523c41a982425e276b24271 🏂
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 9658d0dc17c270138523
...
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1671159616)
re-ACK 9658d0dc17c270138523c41a982425e276b24271 🏂
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 9658d0dc17c270138523
...
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1671166932)
I'll take a stab at rebasing on the bigger thing. It makes sense to do that and then unrebase / debase(?) it later.
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1671166932)
I'll take a stab at rebasing on the bigger thing. It makes sense to do that and then unrebase / debase(?) it later.
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288355467)
I could also imagine a third alternative (3) std::shared_ptr/raw pointer, so that everything is a pointer, but there are still different types for each lifetime. (Basically only replacing std::optional in the current approach with std::shared_ptr)
But this also may be more confusing than it helping.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288355467)
I could also imagine a third alternative (3) std::shared_ptr/raw pointer, so that everything is a pointer, but there are still different types for each lifetime. (Basically only replacing std::optional in the current approach with std::shared_ptr)
But this also may be more confusing than it helping.
📝 achow101 opened a pull request: "Break up script/standard.{h/cpp}"
(https://github.com/bitcoin/bitcoin/pull/28244)
Some future work needs to touch things in script/standard.{h/cpp}, however it is unclear if it is safe to do so as they are included in several different places that could effect standardness and consensus. It contains a mix of policy parameters, consensus parameters, and utilities only used by the wallet. This PR breaks up the various components and renames the files to clearly separate everything.
* `CTxDestination` is moved to a new file `src/addresstype.{cpp/h}`
* `TaprootSpendData` and
...
(https://github.com/bitcoin/bitcoin/pull/28244)
Some future work needs to touch things in script/standard.{h/cpp}, however it is unclear if it is safe to do so as they are included in several different places that could effect standardness and consensus. It contains a mix of policy parameters, consensus parameters, and utilities only used by the wallet. This PR breaks up the various components and renames the files to clearly separate everything.
* `CTxDestination` is moved to a new file `src/addresstype.{cpp/h}`
* `TaprootSpendData` and
...
💬 MarnixCroes commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1671178937)
Should this be added to the release notes?
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1671178937)
Should this be added to the release notes?
💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1288363762)
But `uint256 b = Txid{}` is still allowed? Obviously here it is obvious. However, is it obvious when not on a single line?
```cpp
struct A{
uint256 a; // wtxid
}
void foo(A&a, Txid&t){
a=t;
}
```
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1288363762)
But `uint256 b = Txid{}` is still allowed? Obviously here it is obvious. However, is it obvious when not on a single line?
```cpp
struct A{
uint256 a; // wtxid
}
void foo(A&a, Txid&t){
a=t;
}
```
💬 fanquake commented on pull request "ci: Move tidy to persistent worker":
(https://github.com/bitcoin/bitcoin/pull/28214#issuecomment-1671185391)
> 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.
Why can't this just be done in the QML repo if it's required there?
(https://github.com/bitcoin/bitcoin/pull/28214#issuecomment-1671185391)
> 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.
Why can't this just be done in the QML repo if it's required there?