π¬ hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946377084)
We open/close sockets, we don't start/join them, so I prefer the current name over adding "And".
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946377084)
We open/close sockets, we don't start/join them, so I prefer the current name over adding "And".
π¬ hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946381130)
Ah, so it's done as a general rule even if it makes less logical sense for the function at the root of a thread (as nothing can have taken the lock before it unless the thread is wrapped in some kind of utility that is weird enough to take locks).
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946381130)
Ah, so it's done as a general rule even if it makes less logical sense for the function at the root of a thread (as nothing can have taken the lock before it unless the thread is wrapped in some kind of utility that is weird enough to take locks).
π¬ fanquake commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2642699952)
> Of course would be nice to test this on windows but it should work
@hebasto ?
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2642699952)
> Of course would be nice to test this on windows but it should work
@hebasto ?
π¬ 0xB10C commented on pull request "ci: Bump fuzz task timeout":
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642711802)
fwiw: duration of the `fuzzer,address,undefined,integer, no depends` task over the last few months in MM:SS
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642711802)
fwiw: duration of the `fuzzer,address,undefined,integer, no depends` task over the last few months in MM:SS
π ryanofsky opened a pull request: "multiprocess: Lock CapnpProtocol::m_loop with mutex"
(https://github.com/bitcoin/bitcoin/pull/31815)
This change is intended to fix intermittent failures in `ipc_tests` unit test and `rpc_misc.py` functional tests that happen on mac os, which sometimes fail with error `terminating due to uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument` as reported https://github.com/chaincodelabs/libmultiprocess/issues/154
This error could happen when [`m_loop.reset();`](https://github.com/bitcoin/bitcoin/blob/81eb6cc2c609b2fb3af53a53749e6162fecb34e2/src/ipc/capnp/proto
...
(https://github.com/bitcoin/bitcoin/pull/31815)
This change is intended to fix intermittent failures in `ipc_tests` unit test and `rpc_misc.py` functional tests that happen on mac os, which sometimes fail with error `terminating due to uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument` as reported https://github.com/chaincodelabs/libmultiprocess/issues/154
This error could happen when [`m_loop.reset();`](https://github.com/bitcoin/bitcoin/blob/81eb6cc2c609b2fb3af53a53749e6162fecb34e2/src/ipc/capnp/proto
...
π€ Sjors reviewed a pull request: "Implement BIP 370 PSBTv2"
(https://github.com/bitcoin/bitcoin/pull/21283#pullrequestreview-2601346881)
Studied the other commits as well.
Perhaps there's an opportunity to split this PR by first introducing the internal refactoring that anticipates PSBTv2. But other that I don't think it can be made much smaller.
(https://github.com/bitcoin/bitcoin/pull/21283#pullrequestreview-2601346881)
Studied the other commits as well.
Perhaps there's an opportunity to split this PR by first introducing the internal refactoring that anticipates PSBTv2. But other that I don't think it can be made much smaller.
π¬ Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946295044)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", nit: calling this param `psbt_version` makes it clear that inputs and outputs don't have their own versioning. And consistent with the eventual variable name.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946295044)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", nit: calling this param `psbt_version` makes it clear that inputs and outputs don't have their own versioning. And consistent with the eventual variable name.
π¬ Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946298433)
In https://github.com/bitcoin/bitcoin/commit/2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", ΞΌnit `const`.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946298433)
In https://github.com/bitcoin/bitcoin/commit/2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", ΞΌnit `const`.
π¬ Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946310826)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version": somewhere later in the PR, we should add a target for version 2?
Or better: any version, which would detect that the `PSBTInput` constructor doesn't prevent version 1 (not currently a problem, but could creep in if we build functionality to add inputs to a PSBT).
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946310826)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version": somewhere later in the PR, we should add a target for version 2?
Or better: any version, which would detect that the `PSBTInput` constructor doesn't prevent version 1 (not currently a problem, but could creep in if we build functionality to add inputs to a PSBT).
π¬ Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946376405)
In c6743bf7137615eb20145c6eb3e2da5c82903b1e "Enforce PSBT version constraints": this is hard to read. Would prefer two separate checks based on the PSBT version.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946376405)
In c6743bf7137615eb20145c6eb3e2da5c82903b1e "Enforce PSBT version constraints": this is hard to read. Would prefer two separate checks based on the PSBT version.
π¬ Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946305833)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", nit: `/*psbt_version=*/` would be useful here, but this goes away in a later commit (and before the default is changed to 2).
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946305833)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", nit: `/*psbt_version=*/` would be useful here, but this goes away in a later commit (and before the default is changed to 2).
π¬ Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946391356)
In 0c459e43ba1ce74d2e88dde352e28a63743f4746 "Add PSBT::CacheUnsignedTxPieces": this seems fine, but it would be good to have a method that verifies that for v0 PSBTs these two representations are in sync. You could then enable that method in debug builds and call it in a bunch of places.
The could e.g call `GetUnsignedTx()` (introduced later), skip its `if (tx != std::nullopt)` early return, and compare.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946391356)
In 0c459e43ba1ce74d2e88dde352e28a63743f4746 "Add PSBT::CacheUnsignedTxPieces": this seems fine, but it would be good to have a method that verifies that for v0 PSBTs these two representations are in sync. You could then enable that method in debug builds and call it in a bunch of places.
The could e.g call `GetUnsignedTx()` (introduced later), skip its `if (tx != std::nullopt)` early return, and compare.
π¬ Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946401139)
In 162d0916774c719f1e3749b0d4d204e296ee4ac8 "Add PSBT::GetUniqueID": I think this should check the version number, and then `Assume(version == 0 || tx == std::nullopt)`
The way it's written now suggests `tx` is just a cache that might be stale.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946401139)
In 162d0916774c719f1e3749b0d4d204e296ee4ac8 "Add PSBT::GetUniqueID": I think this should check the version number, and then `Assume(version == 0 || tx == std::nullopt)`
The way it's written now suggests `tx` is just a cache that might be stale.
π¬ Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946413388)
In aeb9e44bc5a221553d2010752c76f8f16205551c "Change PSBT::AddInput to take just PSBTInput": I think it's more readable to just check the PSBT version directly.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946413388)
In aeb9e44bc5a221553d2010752c76f8f16205551c "Change PSBT::AddInput to take just PSBTInput": I think it's more readable to just check the PSBT version directly.
π¬ Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946386141)
In cef9de00f4c2d4c9c033e7dce2b7ffac2e694a26 "Call CacheUnsignedTxPieces in PSBT constructor": might as well do this in the previous commit. Ditto for the next commit.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946386141)
In cef9de00f4c2d4c9c033e7dce2b7ffac2e694a26 "Call CacheUnsignedTxPieces in PSBT constructor": might as well do this in the previous commit. Ditto for the next commit.
β οΈ jsarenik opened an issue: "Memory-only wallet (for faucets)"
(https://github.com/bitcoin/bitcoin/issues/31816)
### Please describe the feature you'd like to see added.
Currently I do following regularly every couple of days in order to refresh faucet wallets and reclaim megabytes of disk space:
```sh
cd ~/.bitcoin/signet/wallets
bitcoin-cli -signet createwallet name false true
# blank with private-keys enabled
cd name
dt=../listdescriptors-true.txt
A=$(sed 's/"timestamp".*$/"timestamp":"now",/' $dt | jq -rc .descriptors)
bitcoin-cli importdescriptors $A
```
It works well for [me](https://alt.signetfau
...
(https://github.com/bitcoin/bitcoin/issues/31816)
### Please describe the feature you'd like to see added.
Currently I do following regularly every couple of days in order to refresh faucet wallets and reclaim megabytes of disk space:
```sh
cd ~/.bitcoin/signet/wallets
bitcoin-cli -signet createwallet name false true
# blank with private-keys enabled
cd name
dt=../listdescriptors-true.txt
A=$(sed 's/"timestamp".*$/"timestamp":"now",/' $dt | jq -rc .descriptors)
bitcoin-cli importdescriptors $A
```
It works well for [me](https://alt.signetfau
...
π¬ maflcko commented on pull request "ci: Bump fuzz task timeout":
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642789552)
Interesting. For context, the "speedup" after Jan 12 is likely due to fuzz input invalidation after commit 37af8bfb34d685bfd5d9a9ba6b35b4705e021535. And the "slowdown" after Dec 13th is likely due to adding fuzz inputs after commit https://github.com/bitcoin-core/qa-assets/commit/5c026e2a97b5f447a863734fb1c2566c33738d01.
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642789552)
Interesting. For context, the "speedup" after Jan 12 is likely due to fuzz input invalidation after commit 37af8bfb34d685bfd5d9a9ba6b35b4705e021535. And the "slowdown" after Dec 13th is likely due to adding fuzz inputs after commit https://github.com/bitcoin-core/qa-assets/commit/5c026e2a97b5f447a863734fb1c2566c33738d01.
π¬ vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946505928)
Maybe it is worth doing this: https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2485715952. It is a super good change and normally I would do it, but the reason I didn't is that it will increase the size of this PR which, I am afraid, would turn reviewers away.
I will proceed to other suggestions and give this some thought...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946505928)
Maybe it is worth doing this: https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2485715952. It is a super good change and normally I would do it, but the reason I didn't is that it will increase the size of this PR which, I am afraid, would turn reviewers away.
I will proceed to other suggestions and give this some thought...
π¬ vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946520524)
Done, but used `uint8_t` because `ReceiveMsgBytes()` (in `master`) takes a span of that and conversion is not possible.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946520524)
Done, but used `uint8_t` because `ReceiveMsgBytes()` (in `master`) takes a span of that and conversion is not possible.
π¬ maflcko commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946527624)
> conversion is not possible.
It should be possible to convert std::byte* to uint8_t* (and vice-versa). The two are the almost the same anyway (https://en.cppreference.com/w/cpp/types/byte). And a span is just a pointer+size, so conversion between the two span types should also be possible.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946527624)
> conversion is not possible.
It should be possible to convert std::byte* to uint8_t* (and vice-versa). The two are the almost the same anyway (https://en.cppreference.com/w/cpp/types/byte). And a span is just a pointer+size, so conversion between the two span types should also be possible.