💬 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_r1295647557)
I guess for this repo, it should be sufficient to only have a `master` cache (written only by `master`) and all pull requests and other branches simply read-only from `master`. This should make the storage needs constant (limited) and easier to think about, without much downside.

💬 1ma commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1680299138)
> If P2MS becomes inefficient or difficult to relay, the next logical choice would be P2PK. This would lead to an even faster inflation of the UTXO set.

Then the next logical step is also making P2PK non-standard (preferably on the same release), as suggested by @Semisol in this thread.

💬 RobinLinus commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1680300119)
> won't find the overhead of using P2PK problematic

P2PK is more costly, which reduces the toxic waste spammers can inscribe per Dollar. However, as said before, P2PK should be nonstandard too.

👍 MarcoFalke approved a pull request: "Break up script/standard.{h/cpp}"
(https://github.com/bitcoin/bitcoin/pull/28244#pullrequestreview-1580239988)
ACK 91d924ede1b421df31c895f4f43359e453a09ca5 😇

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 91d924ede1b421df31c895f4f4
...

💬 MarcoFalke commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1295681864)
7a172c76d2361fc3cdf6345590e26c79a7821672: nit: Any reason to put this in this scope, instead of in the only scope that uses it (one function)?

Also, could use `using` (C++11)?

💬 MarcoFalke commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1295665496)
unrelated: IIUC this creates a useless copy. Might be better to just retain the reference.

💬 MarcoFalke commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1295682450)
7a172c76d2361fc3cdf6345590e26c79a7821672: nit:

```
addresstype.cpp should add these lines:
#include <cassert>
#include "crypto/sha256.h" // for CSHA256
#include "span.h" // for Span

💬 MarcoFalke commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1295695839)
nit in 7a172c76d2361fc3cdf6345590e26c79a7821672: Could keep this in the `script` directory, next to `script/descriptor.cpp`?

💬 MarcoFalke commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1295692671)
7a172c76d2361fc3cdf6345590e26c79a7821672:nit: missing newline?

💬 MarcoFalke commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1295691564)
bacdb2e208531124e85ed2d4ea2a4b508fbb5088: nit:

```
script/solver.cpp should add these lines:
#include <cassert> // for assert
#include "prevector.h" // for operator-, prevector

💬 MarcoFalke commented on pull request "test: check backup from `migratewallet` can be successfully restored":
(https://github.com/bitcoin/bitcoin/pull/28257#issuecomment-1680357309)
lgtm ACK 769f5b15f207ce6d1067672ea5e195541c97de6b

💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1680366922)
@achow101 @ariard See [[bitcoin-dev] Full-RBF testing: at least 31% of hash power, over at least 4 pools, is mining full-RBF right now](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-August/021890.html) for a re-confirmation of my OTS calendar findings, including a tool that anyone can easily use to also test full-RBF adoption for themselves.

@Daniel600
> The methodology of 37% assessment above is not thorough - a simple test of representative trx count with a time gap follow
...

💬 MarcoFalke commented on pull request "ci: Fix macOS-cross SDK rsync":
(https://github.com/bitcoin/bitcoin/pull/28273#issuecomment-1680382184)
Confirmed to be fixed now, see https://cirrus-ci.com/task/5805313979318272?logs=base_depends_built#L4

👍 hebasto approved a pull request: "ci: Fix macOS-cross SDK rsync"
(https://github.com/bitcoin/bitcoin/pull/28273#pullrequestreview-1580323954)
ACK fa6e5d3eeffebf81b5d7ca99bf7b5e70356516ab

💬 real-or-random 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_r1295724511)
You, mean also for the Docker build cache? Yeah, that's a damn simple approach. Then 10 GB are enough for sure, and okay, PRs that modify the Dockerfile will rebuild the Docker image on every push time, but seems acceptable. Anyway, if working on the images, it makes sense to test/build them locally first.

But wouldn't you need artifacts again to transfer the image from the Docker build task to the actual CI task, namely in PRs that modify the Dockerfile, to avoid building the image in ever
...

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1295738148)
I stared at this a bit and realized it does not make sense without the `ResetIbd()` call and further, the point is to jump out of IBD at some point later during the test (by changing the mock time so that it is near or before the chain tip). Added that and some comments.

💬 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_r1295738785)
> Or is this acceptable for this repo, because there won't be many tasks?

There shouldn't be any difference for this repo, because every task has its own image.

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1680410324)
`ca7a9983eb...ae75114975`: enter IBD at the start and possibly jump out at some point, like in `src/test/fuzz/process_messages.cpp`.

💬 MarcoFalke commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1295772826)
@theuni You'll have to share exact steps to reproduce, ideally starting from a fresh install of you operating system. Otherwise it will be close to impossible to reproduce. Locally and on CI this isn't caught (otherwise CI would be red). c.f

```
$ (cd src/ && clang-tidy --load=/home/fake/bitcoin/contrib/devtools/bitcoin-tidy/build/libbitcoin-tidy.so -checks="-*,bitcoin-*" wallet/wallet.cpp )
12 warnings generated.
Suppressed 12 warnings (12 in non-user code).
Use -header-filter=.* to di
...

🚀 fanquake merged a pull request: "test: check backup from `migratewallet` can be successfully restored"
(https://github.com/bitcoin/bitcoin/pull/28257)