💬 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_r1293347851)
> So every image will be re-built every week regardless of whether it changed?

Sorry, I don't follow. Why will it be re-built every week?

💬 MarcoFalke commented on pull request "Unified mixture of unnamed namespace and static.":
(https://github.com/bitcoin/bitcoin/pull/28259#issuecomment-1677164168)
Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

* Time spent on review
* Accidental introduction of bugs
* (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

For more in
...

✅ MarcoFalke closed a pull request: "Unified mixture of unnamed namespace and static."
(https://github.com/bitcoin/bitcoin/pull/28259)

💬 MarcoFalke commented on pull request "Unified mixture of unnamed namespace and static.":
(https://github.com/bitcoin/bitcoin/pull/28259#issuecomment-1677165010)
(Also the `refactor:` in the title was missing, according to the dev notes)

💬 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?

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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?

💬 samyan commented on issue "Regtest mode loses unspents after day":
(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>
```

🚀 fanquake merged a pull request: "bitcoin-tidy: fix macOS build"
(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

💬 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
...

💬 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
...

💬 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.

💬 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.