💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439389706)
```suggestion
sudo xcode-select --switch "/Applications/Xcode15.1.app"
```
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439389706)
```suggestion
sudo xcode-select --switch "/Applications/Xcode15.1.app"
```
💬 fanquake commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439390908)
If any version is going to be picked, shouldn't it be the one we use for releases?
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439390908)
If any version is going to be picked, shouldn't it be the one we use for releases?
💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439391581)
Or `sudo xcode-select -switch /Applications/Xcode_15.1.app`. It works in https://github.com/hebasto/bitcoin/actions/runs/7385516810/job/20090426555
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439391581)
Or `sudo xcode-select -switch /Applications/Xcode_15.1.app`. It works in https://github.com/hebasto/bitcoin/actions/runs/7385516810/job/20090426555
💬 fanquake commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439391984)
> Run sudo xcode-select --switch "/Applications/Xcode15.1.app"
xcode-select: error: invalid developer directory '/Applications/Xcode15.1.app'
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439391984)
> Run sudo xcode-select --switch "/Applications/Xcode15.1.app"
xcode-select: error: invalid developer directory '/Applications/Xcode15.1.app'
💬 darosior commented on pull request "fuzz: avoid underflow in `coins_view` target":
(https://github.com/bitcoin/bitcoin/pull/29164#issuecomment-1873966765)
Sigh... I'm tired of this target just doing random things. Turns out you can't perform the sanity checks because it's using a dummy backend where `Flush()` won't actually erase the cache. I'll just use a hack to avoid the crash in #28216.
(https://github.com/bitcoin/bitcoin/pull/29164#issuecomment-1873966765)
Sigh... I'm tired of this target just doing random things. Turns out you can't perform the sanity checks because it's using a dummy backend where `Flush()` won't actually erase the cache. I'll just use a hack to avoid the crash in #28216.
✅ darosior closed a pull request: "fuzz: avoid underflow in `coins_view` target"
(https://github.com/bitcoin/bitcoin/pull/29164)
(https://github.com/bitcoin/bitcoin/pull/29164)
💬 darosior commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-1873968224)
Removed the use of a proper fix to simply abuse `Flush()` when the exception is hit to avoid the crash. See the first commit.
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-1873968224)
Removed the use of a proper fix to simply abuse `Flush()` when the exception is hit to avoid the crash. See the first commit.
💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439419407)
> If any version is going to be picked, shouldn't it be the one we use for releases?
Xcode 15.0.1 (15A507) and 15.1 (15C65) versions are available now, and they are subjects for frequent updates.
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439419407)
> If any version is going to be picked, shouldn't it be the one we use for releases?
Xcode 15.0.1 (15A507) and 15.1 (15C65) versions are available now, and they are subjects for frequent updates.
💬 fanquake commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439422328)
> Xcode 15.0.1 (15A507) and 15.1 (15C65) versions are available now,
My suggestion was to pick `15.0`, which we use for release builds, so that CI better matches our release process, rather than `15.1`, as you suggested. Otherwise code could compile in the CI, but not in Guix.
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439422328)
> Xcode 15.0.1 (15A507) and 15.1 (15C65) versions are available now,
My suggestion was to pick `15.0`, which we use for release builds, so that CI better matches our release process, rather than `15.1`, as you suggested. Otherwise code could compile in the CI, but not in Guix.
💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439433156)
Looks like the macOS CI now fails for unrelated reasons. If someone created a separate pull request to bump xcode in the macOS CI, that'd be great.
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1439433156)
Looks like the macOS CI now fails for unrelated reasons. If someone created a separate pull request to bump xcode in the macOS CI, that'd be great.
💬 brunoerg commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-1874011124)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-1874011124)
Concept ACK
💬 maflcko commented on pull request "miniscript: make operator""_mst consteval":
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-1874012428)
Confirmed that this cuts a third of the runtime off of the fuzz input from https://github.com/bitcoin/bitcoin/pull/28832#issuecomment-1827538761
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-1874012428)
Confirmed that this cuts a third of the runtime off of the fuzz input from https://github.com/bitcoin/bitcoin/pull/28832#issuecomment-1827538761
💬 fanquake commented on pull request "miniscript: make operator""_mst consteval":
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-1874014418)
Maybe we should also retry the straight constexpr -> consteval change, without having to do all the refactoring for MSVC? Microsoft might have fixed their compiler.
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-1874014418)
Maybe we should also retry the straight constexpr -> consteval change, without having to do all the refactoring for MSVC? Microsoft might have fixed their compiler.
👍 shaavan approved a pull request: "refactor: Allow std::span construction from CKey"
(https://github.com/bitcoin/bitcoin/pull/29133#pullrequestreview-1800426197)
Code Review ACK
<details>
<summary>
<b>Notes:</b>
</summary>
**Commit-1:**
Since **`std::span`** requires a mutable pointer to the data for certain operations, this change makes XOnlyPubKey compatible with std::span.
**Commit-2:**
Since the return type of `begin()` and `end()` much match the data type of the `std::span` of the container, this change makes CKey compatible with `std::span`, and allows `std::span<std::byte>` construction for it.
**Commit-3:**
Tests t
...
(https://github.com/bitcoin/bitcoin/pull/29133#pullrequestreview-1800426197)
Code Review ACK
<details>
<summary>
<b>Notes:</b>
</summary>
**Commit-1:**
Since **`std::span`** requires a mutable pointer to the data for certain operations, this change makes XOnlyPubKey compatible with std::span.
**Commit-2:**
Since the return type of `begin()` and `end()` much match the data type of the `std::span` of the container, this change makes CKey compatible with `std::span`, and allows `std::span<std::byte>` construction for it.
**Commit-3:**
Tests t
...
💬 maflcko commented on pull request "refactor: Allow std::span construction from CKey":
(https://github.com/bitcoin/bitcoin/pull/29133#issuecomment-1874030203)
> If we now have `std::span` since C++20, are we thinking of making every container in the codebase `std::span` compatible and moving away from needing to use our ### custom `Span` class?
Yes, I think it makes sense to allow new code to use `std::span` directly. Also, I think it makes sense to (longer term) remove `Span` (or make it an alias of `std::span`) to avoid confusion which to use in new code, and to get the additional compile-time checks from `std::span`.
(https://github.com/bitcoin/bitcoin/pull/29133#issuecomment-1874030203)
> If we now have `std::span` since C++20, are we thinking of making every container in the codebase `std::span` compatible and moving away from needing to use our ### custom `Span` class?
Yes, I think it makes sense to allow new code to use `std::span` directly. Also, I think it makes sense to (longer term) remove `Span` (or make it an alias of `std::span`) to avoid confusion which to use in new code, and to get the additional compile-time checks from `std::span`.
🤔 glozow reviewed a pull request: "test: doc: follow-up #28368"
(https://github.com/bitcoin/bitcoin/pull/29013#pullrequestreview-1800463253)
utACK b1318dcc56a0181783ee7ddbd388ae878a0efc52
(https://github.com/bitcoin/bitcoin/pull/29013#pullrequestreview-1800463253)
utACK b1318dcc56a0181783ee7ddbd388ae878a0efc52
⚠️ zhaocaipeng opened an issue: "is a getBlockTemplate transactions's bug?"
(https://github.com/bitcoin/bitcoin/issues/29166)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I invoke the getBlockTemplate interface using SegWit. The sum of weights in the returned transactions data is 3991625, and the sum of SigOps is 20037. Each time, I package all the transactions returned by getBlockTemplate into a block. I observe that the weight limit is 4000000, and the data returned complies with this requirement. However, I notice that the SigOp limit is 80000, and it is
...
(https://github.com/bitcoin/bitcoin/issues/29166)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I invoke the getBlockTemplate interface using SegWit. The sum of weights in the returned transactions data is 3991625, and the sum of SigOps is 20037. Each time, I package all the transactions returned by getBlockTemplate into a block. I observe that the weight limit is 4000000, and the data returned complies with this requirement. However, I notice that the SigOp limit is 80000, and it is
...
💬 stickies-v commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439417050)
nit: Perhaps useful to parameterize the host platform to highlight it's configurable? E.g.
```sh
HOST_PLATFORM="x86_64-pc-linux-gnu"
cd <BITCOIN_SOURCE_DIRECTORY>
make -C depends NO_QT=1 MULTIPROCESS=1
CONFIG_SITE=$PWD/depends/$HOST_PLATFORM/share/config.site ./configure
```
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439417050)
nit: Perhaps useful to parameterize the host platform to highlight it's configurable? E.g.
```sh
HOST_PLATFORM="x86_64-pc-linux-gnu"
cd <BITCOIN_SOURCE_DIRECTORY>
make -C depends NO_QT=1 MULTIPROCESS=1
CONFIG_SITE=$PWD/depends/$HOST_PLATFORM/share/config.site ./configure
```
💬 stickies-v commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439454381)
dead link
```suggestion
In the meantime, the developer guide [Internal interface guidelines](../developer-notes.md#internal-interface-guidelines) can provide guidance on keeping interfaces consistent and functional and avoiding compile errors.
```
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439454381)
dead link
```suggestion
In the meantime, the developer guide [Internal interface guidelines](../developer-notes.md#internal-interface-guidelines) can provide guidance on keeping interfaces consistent and functional and avoiding compile errors.
```
💬 stickies-v commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439476415)
nit
```suggestion
- **client class (in generated code)**: A C++ class generated from a Cap’n Proto interface which inherits from a Bitcoin Core abstract class, and implements each virtual method to send IPC requests to another process. (see also [components section](#c-client-subclasses-in-generated-code))
```
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439476415)
nit
```suggestion
- **client class (in generated code)**: A C++ class generated from a Cap’n Proto interface which inherits from a Bitcoin Core abstract class, and implements each virtual method to send IPC requests to another process. (see also [components section](#c-client-subclasses-in-generated-code))
```