💬 achow101 commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2342296606)
In 4d07eff13002384ed59a3b7f592be56a25b88c15 "descriptor: ToPrivateString() pass if at least 1 priv key
`has_priv_key` is a misnomer since for public strings, it has a completely different meaning. I would prefer naming this something more generic, like `any_success`, here and elsewhere.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2342296606)
In 4d07eff13002384ed59a3b7f592be56a25b88c15 "descriptor: ToPrivateString() pass if at least 1 priv key
`has_priv_key` is a misnomer since for public strings, it has a completely different meaning. I would prefer naming this something more generic, like `any_success`, here and elsewhere.
💬 achow101 commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2342299196)
In 4d07eff13002384ed59a3b7f592be56a25b88c15 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
nit: can be simplified to
```suggestion
return type != StringType::PRIVATE
```
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2342299196)
In 4d07eff13002384ed59a3b7f592be56a25b88c15 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
nit: can be simplified to
```suggestion
return type != StringType::PRIVATE
```
💬 achow101 commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2342300386)
In 4d07eff13002384ed59a3b7f592be56a25b88c15 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
`has_priv_key` does not need to be a pointer here.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2342300386)
In 4d07eff13002384ed59a3b7f592be56a25b88c15 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
`has_priv_key` does not need to be a pointer here.
🚀 achow101 merged a pull request: "kernel: make blockTip index const"
(https://github.com/bitcoin/bitcoin/pull/33321)
(https://github.com/bitcoin/bitcoin/pull/33321)
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2342348714)
We want to get the private key early so that we know whether we will be able to even do anything here.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2342348714)
We want to get the private key early so that we know whether we will be able to even do anything here.
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2342368768)
I actually prefer verbosity in type names.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2342368768)
I actually prefer verbosity in type names.
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2342376993)
For a followup or if I need to retouch.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2342376993)
For a followup or if I need to retouch.
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3282694791)
> It would be helpful to mention why this is done in the commit message
If I need to retouch.
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3282694791)
> It would be helpful to mention why this is done in the commit message
If I need to retouch.
💬 achow101 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282710122)
ACK 653a9849d5f98ba80e334ddc0ae9a5e367459f59
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282710122)
ACK 653a9849d5f98ba80e334ddc0ae9a5e367459f59
🚀 achow101 merged a pull request: "common: Make arith_uint256 trivially copyable"
(https://github.com/bitcoin/bitcoin/pull/33332)
(https://github.com/bitcoin/bitcoin/pull/33332)
💬 davidgumberg commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282744720)
The members of `base_uint` are all [trivially copyable](https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable.html): `WIDTH` is a scalar, and `pn` is an array of scalars, so the class is trivially copyable and we should use the implicit copy constructors of the class. But, by explicitly declaring the implicitly defined copy constructors the move constructors are suppressed.
<details><summary>
#### (https://en.cppreference.com/w/cpp/language/move_constructor.html)
</summary>
...
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282744720)
The members of `base_uint` are all [trivially copyable](https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable.html): `WIDTH` is a scalar, and `pn` is an array of scalars, so the class is trivially copyable and we should use the implicit copy constructors of the class. But, by explicitly declaring the implicitly defined copy constructors the move constructors are suppressed.
<details><summary>
#### (https://en.cppreference.com/w/cpp/language/move_constructor.html)
</summary>
...
💬 Raimo33 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282754152)
if I understand this correctly, move constructors were suppressed before this PR as well.
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282754152)
if I understand this correctly, move constructors were suppressed before this PR as well.
💬 davidgumberg commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282762069)
> if I understand this correctly, move constructors were suppressed before this PR as well.
They were, so this is not a regression, but it would be better for the implicitly defined move constructors to be usable.
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282762069)
> if I understand this correctly, move constructors were suppressed before this PR as well.
They were, so this is not a regression, but it would be better for the implicitly defined move constructors to be usable.
💬 Raimo33 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282765661)
> it would be better for the implicitly defined move constructors to be usable.
good catch, totally agree, let's run some benchmarks
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282765661)
> it would be better for the implicitly defined move constructors to be usable.
good catch, totally agree, let's run some benchmarks
💬 achow101 commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3282783147)
ACK 3bdff9a154733f8f9f379448f5595a2e90474bc6
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3282783147)
ACK 3bdff9a154733f8f9f379448f5595a2e90474bc6
💬 achow101 commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#issuecomment-3282836726)
ACK fa96a4afea2a9bf90c843198e75a00acef02c32d
(https://github.com/bitcoin/bitcoin/pull/33243#issuecomment-3282836726)
ACK fa96a4afea2a9bf90c843198e75a00acef02c32d
🚀 achow101 merged a pull request: "test: Fix CLI_MAX_ARG_SIZE issues"
(https://github.com/bitcoin/bitcoin/pull/33243)
(https://github.com/bitcoin/bitcoin/pull/33243)
💬 151henry151 commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3282884135)
I see that there were two failing checks -- is there any further suggested action I could take to help with this? I'm beginning to investigate now to try to understand what failed and why.
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3282884135)
I see that there were two failing checks -- is there any further suggested action I could take to help with this? I'm beginning to investigate now to try to understand what failed and why.
💬 achow101 commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-3282946065)
@laanwj @casey Are either of you still working on a PR for this?
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-3282946065)
@laanwj @casey Are either of you still working on a PR for this?
💬 davidgumberg commented on pull request "cmake: fatal error when PIE not supported":
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3283024096)
> > I think the current behaviour of using PIE for all targets is sensible
>
> I am not convinced and I really would like to know the rationale for that. Maybe @hebasto or @theuni knows?
For motivation, you can look to the commit which enabled PIE by default: https://github.com/bitcoin/bitcoin/commit/3f94dfa25fc1b0e838d368a9b2683a634c.
In order for operating systems employing ASLR to randomize the base address of an executable's `text` segment, binaries must be built as PIE, and this be
...
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3283024096)
> > I think the current behaviour of using PIE for all targets is sensible
>
> I am not convinced and I really would like to know the rationale for that. Maybe @hebasto or @theuni knows?
For motivation, you can look to the commit which enabled PIE by default: https://github.com/bitcoin/bitcoin/commit/3f94dfa25fc1b0e838d368a9b2683a634c.
In order for operating systems employing ASLR to randomize the base address of an executable's `text` segment, binaries must be built as PIE, and this be
...