💬 MarcoFalke commented on pull request "ci: Run "macOS 11.0 [gui, no tests] [jammy]" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293356505)
Ah, sorry, I assumed cache entries will be unconditionally deleted after 7 days. New question: So every image will be re-built regardless of whether it changed, given 10GB of non-image cache items (ccache+depends+...) have been created?
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293356505)
Ah, sorry, I assumed cache entries will be unconditionally deleted after 7 days. New question: So every image will be re-built regardless of whether it changed, given 10GB of non-image cache items (ccache+depends+...) have been created?
💬 theStack commented on pull request "test: refactor: support sending funds with outpoint result":
(https://github.com/bitcoin/bitcoin/pull/28264#discussion_r1293357213)
SGTM to make it a more generic helper that supports multiple outputs / returns multiple UTXOs (need to use `send` instead of `sendtoaddress`, but that shouldn't matter). With that, even more instances of `find_vout_address` can be replaced. Moving the helper to TestNode wouldn't be a good fit, as the outpoints creations not only operate on TestNode objects, but in some instances also on `RPCOverloadWrapper`s (result from `get_wallet_rpc`). Moving it to TestFramework should work though.
(https://github.com/bitcoin/bitcoin/pull/28264#discussion_r1293357213)
SGTM to make it a more generic helper that supports multiple outputs / returns multiple UTXOs (need to use `send` instead of `sendtoaddress`, but that shouldn't matter). With that, even more instances of `find_vout_address` can be replaced. Moving the helper to TestNode wouldn't be a good fit, as the outpoints creations not only operate on TestNode objects, but in some instances also on `RPCOverloadWrapper`s (result from `get_wallet_rpc`). Moving it to TestFramework should work though.
💬 hebasto commented on pull request "ci: Run "macOS 11.0 [gui, no tests] [jammy]" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293360199)
> New question: So every image will be re-built regardless of whether it changed...
If no changes were introduced, the image is built from the cached layers.
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293360199)
> New question: So every image will be re-built regardless of whether it changed...
If no changes were introduced, the image is built from the cached layers.
💬 MarcoFalke commented on pull request "ci: Run "macOS 11.0 [gui, no tests] [jammy]" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293361403)
But the cached layers will be evicted after 10GB of (let's say) ccache items.
> the cache eviction policy will create space by deleting the oldest caches in the repository.
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293361403)
But the cached layers will be evicted after 10GB of (let's say) ccache items.
> the cache eviction policy will create space by deleting the oldest caches in the repository.
💬 fanquake commented on pull request "guix: use GCC 12.3.0 to build releases":
(https://github.com/bitcoin/bitcoin/pull/27897#issuecomment-1677177179)
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65151 landed in Guix. So I've updated the time-machine to include that change, and removed the commit dropping supported for `powerpc64-linux-gnu`. The last TODO here is resolve the determinism issue with the powerpc64le-linux-gnu build.
(https://github.com/bitcoin/bitcoin/pull/27897#issuecomment-1677177179)
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65151 landed in Guix. So I've updated the time-machine to include that change, and removed the commit dropping supported for `powerpc64-linux-gnu`. The last TODO here is resolve the determinism issue with the powerpc64le-linux-gnu build.
💬 hebasto commented on pull request "ci: Run "macOS 11.0 [gui, no tests] [jammy]" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293363847)
Yes, it will happen from time to time. But not every run :)
There is an alternative to that approach, which is using Container Registry for images.
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293363847)
Yes, it will happen from time to time. But not every run :)
There is an alternative to that approach, which is using Container Registry for images.
💬 stratospher commented on pull request "crypto: BIP324 ciphersuite follow-up":
(https://github.com/bitcoin/bitcoin/pull/28267#discussion_r1293365684)
done. added it in a new commit.
(https://github.com/bitcoin/bitcoin/pull/28267#discussion_r1293365684)
done. added it in a new commit.
💬 fanquake commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#issuecomment-1677184316)
> Some changes here are also in https://github.com/bitcoin/bitcoin/pull/28008, but either or both can go in.
@sipa could you update the PR dscription in regards to this, now that #28008 has been merged.
cc @stratospher you might be interested in reviewing here?
(https://github.com/bitcoin/bitcoin/pull/28100#issuecomment-1677184316)
> Some changes here are also in https://github.com/bitcoin/bitcoin/pull/28008, but either or both can go in.
@sipa could you update the PR dscription in regards to this, now that #28008 has been merged.
cc @stratospher you might be interested in reviewing here?
💬 samyan commented on issue "Regtest mode loses unspents after day":
(https://github.com/bitcoin/bitcoin/issues/28262#issuecomment-1677188980)
> -mocktim
Still the same.
(https://github.com/bitcoin/bitcoin/issues/28262#issuecomment-1677188980)
> -mocktim
Still the same.
💬 fanquake commented on pull request "crypto: BIP324 ciphersuite follow-up":
(https://github.com/bitcoin/bitcoin/pull/28267#discussion_r1293378301)
Note that we keep our includes, and stardard library headers separate, so `span.h` should be added above. We also try use the cpp headers, [so cstdint over stdint.h](https://en.cppreference.com/w/cpp/header/cstdint) (regardless of what IWYU suggests).
```suggestion
#include <cstdint>
```
(https://github.com/bitcoin/bitcoin/pull/28267#discussion_r1293378301)
Note that we keep our includes, and stardard library headers separate, so `span.h` should be added above. We also try use the cpp headers, [so cstdint over stdint.h](https://en.cppreference.com/w/cpp/header/cstdint) (regardless of what IWYU suggests).
```suggestion
#include <cstdint>
```
🚀 fanquake merged a pull request: "bitcoin-tidy: fix macOS build"
(https://github.com/bitcoin/bitcoin/pull/28258)
(https://github.com/bitcoin/bitcoin/pull/28258)
💬 fanquake commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1293380343)
We can just drop any `// For ...` comments entirely
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1293380343)
We can just drop any `// For ...` comments entirely
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293390811)
> Haven't really looked at implementation yet, but can documentation be updated to say what happens with type mismatches?
This sound like a bug in the source code, so I don't think it makes sense to document unreachable or unexpected code paths. Generally, in the RPC server non-fatal bugs are expected to throw an internal bug message.
I haven't looked at the code, but types of arguments passed in by the user should be checked before *m_fun* is run. *m_fun*, and thus, `Arg()` isn't run when
...
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293390811)
> Haven't really looked at implementation yet, but can documentation be updated to say what happens with type mismatches?
This sound like a bug in the source code, so I don't think it makes sense to document unreachable or unexpected code paths. Generally, in the RPC server non-fatal bugs are expected to throw an internal bug message.
I haven't looked at the code, but types of arguments passed in by the user should be checked before *m_fun* is run. *m_fun*, and thus, `Arg()` isn't run when
...
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293392813)
> I think compile time RPC type checks would require templatising `RPCArg` by what is currently `m_type` (and `m_inner` perhaps), changing `RPCHelpMan` to be templatised based on a `std::tuple<...>` of `RPCArg<X>`s, and likewise for `RPCMethodImpl` which then gives the actual RPC implementation compile time access to its expected argument types.
I haven't looked at this, but some `RPCHelpMan` have run-time inputs, so I am not sure if they can be trivially converted to compile-time inputs. May
...
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293392813)
> I think compile time RPC type checks would require templatising `RPCArg` by what is currently `m_type` (and `m_inner` perhaps), changing `RPCHelpMan` to be templatised based on a `std::tuple<...>` of `RPCArg<X>`s, and likewise for `RPCMethodImpl` which then gives the actual RPC implementation compile time access to its expected argument types.
I haven't looked at this, but some `RPCHelpMan` have run-time inputs, so I am not sure if they can be trivially converted to compile-time inputs. May
...
💬 theStack commented on pull request "test: refactor: support sending funds with outpoint result":
(https://github.com/bitcoin/bitcoin/pull/28264#issuecomment-1677224812)
Force-pushed with the feedback https://github.com/bitcoin/bitcoin/pull/28264#discussion_r1293224263 tackled and adapted the PR description accordingly.
(https://github.com/bitcoin/bitcoin/pull/28264#issuecomment-1677224812)
Force-pushed with the feedback https://github.com/bitcoin/bitcoin/pull/28264#discussion_r1293224263 tackled and adapted the PR description accordingly.
💬 fanquake commented on pull request "build: Pointer Authentication and Branch Target Identification for aarch64 Linux (Guix)":
(https://github.com/bitcoin/bitcoin/pull/24123#issuecomment-1677226187)
Rebased onto #27897, which simplifies the actual changes here, and dropped no-longer needed commits.
(https://github.com/bitcoin/bitcoin/pull/24123#issuecomment-1677226187)
Rebased onto #27897, which simplifies the actual changes here, and dropped no-longer needed commits.
💬 fanquake commented on pull request "build: Pointer Authentication and Branch Target Identification for aarch64 Linux (Guix)":
(https://github.com/bitcoin/bitcoin/pull/24123#discussion_r1293395019)
Made this Guix-only for now.
(https://github.com/bitcoin/bitcoin/pull/24123#discussion_r1293395019)
Made this Guix-only for now.
💬 fanquake commented on pull request "build: LLVM 16 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1677230335)
Re-rebased on #27897, and updated 6f6552688233e35a56f7a4c428c99c657150d313 to include `lld-15` & `lld-as-ld-wrapper-15`. Using lld-as-ld-wrapper seems to be the best way to make build systems (like Qts) work with LLD.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1677230335)
Re-rebased on #27897, and updated 6f6552688233e35a56f7a4c428c99c657150d313 to include `lld-15` & `lld-as-ld-wrapper-15`. Using lld-as-ld-wrapper seems to be the best way to make build systems (like Qts) work with LLD.
💬 fanquake commented on pull request "guix: Use LTO to build releases":
(https://github.com/bitcoin/bitcoin/pull/25391#issuecomment-1677251367)
Rebased on #27897. Simplifed / squashed down the changes here. Updated the PR description. The current (non-conceptual) blocker is that Qt for Windows doesn't really work under LTO... See 8031e400af66bf5267201069a7c062cb6a9d02ea.
(https://github.com/bitcoin/bitcoin/pull/25391#issuecomment-1677251367)
Rebased on #27897. Simplifed / squashed down the changes here. Updated the PR description. The current (non-conceptual) blocker is that Qt for Windows doesn't really work under LTO... See 8031e400af66bf5267201069a7c062cb6a9d02ea.
💬 dergoegge commented on issue "meta: Isolated fuzzing of net processing":
(https://github.com/bitcoin/bitcoin/issues/27502#issuecomment-1677257433)
> What is this going to fuzz exactly? Is this intended to be abstracted away from validation/mempool behaviours (ie, stubbing them out)?
Mostly all code in `net_processing.cpp` (and relevant modules like orphanage, tx tracker) but excluding net, validation or mempool logic. Stubbing the latter out gives us control over testing net processing behavior related to these adjacent modules by being able to mock their behavior and state.
I'm not saying that we shouldn't fuzz the other modules, ju
...
(https://github.com/bitcoin/bitcoin/issues/27502#issuecomment-1677257433)
> What is this going to fuzz exactly? Is this intended to be abstracted away from validation/mempool behaviours (ie, stubbing them out)?
Mostly all code in `net_processing.cpp` (and relevant modules like orphanage, tx tracker) but excluding net, validation or mempool logic. Stubbing the latter out gives us control over testing net processing behavior related to these adjacent modules by being able to mock their behavior and state.
I'm not saying that we shouldn't fuzz the other modules, ju
...