Bitcoin Core Github
44 subscribers
119K links
Download Telegram
🚀 achow101 merged a pull request: "test: Remove polling loop from test_runner (take 2)"
(https://github.com/bitcoin/bitcoin/pull/33141)
💬 achow101 commented on pull request "kernel: make blockTip index const":
(https://github.com/bitcoin/bitcoin/pull/33321#issuecomment-3282473712)
ACK 75d9b72475708ee0da13fb23ef65dcced805b6af
💬 l0rinc commented on pull request "test/refactor: use test deque to avoid quadratic iteration":
(https://github.com/bitcoin/bitcoin/pull/33313#issuecomment-3282473980)
Thanks for the reviews so far, I have rebased it after https://github.com/bitcoin/bitcoin/pull/33141, should be trivial to re-review
👍 hodlinator approved a pull request: "common: Make arith_uint256 trivially copyable"
(https://github.com/bitcoin/bitcoin/pull/33332#pullrequestreview-3213497518)
re-ACK 653a9849d5f98ba80e334ddc0ae9a5e367459f59

New push only adds `static_assert` with motivation.
📝 BrandonOdiwuor opened a pull request: "ci: use Mold linker for asan-lsan-ubsan-integer-no-depends-usdt workflow"
(https://github.com/bitcoin/bitcoin/pull/33370)
Follow up to https://github.com/bitcoin/bitcoin/pull/32888#pullrequestreview-2993523631 and https://github.com/bitcoin/bitcoin/pull/32888#issuecomment-3044773485

>>Can we use `mold` as a linker in other Linux based system workflows ? dependencies [we have](https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#compiler) seem to satisfy the deps here https://github.com/rui314/mold?tab=readme-ov-file#how-to-build
>
> Sure, happy to review a follow-up. Only place to avoid it would
...
💬 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.
💬 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
```
💬 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.
🚀 achow101 merged a pull request: "kernel: make blockTip index const"
(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.
💬 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.
💬 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.
💬 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.
💬 achow101 commented on pull request "common: Make arith_uint256 trivially copyable":
(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)
💬 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>


...
💬 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.
💬 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.
💬 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
💬 achow101 commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3282783147)
ACK 3bdff9a154733f8f9f379448f5595a2e90474bc6