💬 dergoegge commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1657827011)
Concept ACK

💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1657857733)
@ajtowns

> > [Binance Pool, 8.2%](https://mempool.space/tx/64a26f750c7c58812bd475d054a9aa05248b6a8a2d53c0db38c0624197a4c68a)
>
> This replaced 87f20bf4a8b71a255e47876ca7563cdcab1a830490a1b130bbe71c28d8da3884 which had a 3.14sat/vb feerate. statoshi reported minfees up to 3.09sat/vb in the period between the original propagating and the replacement being mined.
>
> > [KuCoinPool, 1.4%](https://mempool.space/tx/8df183b087d433eac4d8e44eca5c7162a2540ee078a28ca04316fd82e12ec435)
>
> This
...

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1657883999)
> The dbwrapper is a key component of the libbitcoinkernel library. Future users of this library would not want to contend with having the leveldb headers exposed and potentially polluting their project's namespace.

Not sure about the motivation. I don't think any user should include the dbwrapper either? No user should be directly writing or reading from the db. The only reason why they'd have to include the header is to get access to `DBOptions`, which would alternatively and trivially be a
...

💬 MarcoFalke commented on pull request "ci: Use documented `CCACHE_MAXSIZE` instead of `CCACHE_SIZE`":
(https://github.com/bitcoin/bitcoin/pull/28188#issuecomment-1657898481)
lgtm ACK 79ceb161dbf7e033ce557d98e297bc3333665f26

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1278954263)
switched to `CallOneOf` and used `NO_THREAD_SAFETY_ANALYSIS`.

👍 MarcoFalke approved a pull request: "ci: Run "macOS native x86_64" job on GitHub Actions"
(https://github.com/bitcoin/bitcoin/pull/28187#pullrequestreview-1554233354)
lgtm, Should squash?

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1278964021)
I think you forgot to skip on the monotree fork?

```
skip: $CIRRUS_REPO_FULL_NAME == "bitcoin-core/gui" && $CIRRUS_PR == "" # No need to run on the read-only mirror, unless it is a PR. https://cirrus-ci.org/guide/writing-tasks/#conditional-task-execution

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1278965679)
Seems fragile. It may be better to set a hard-coded path, like before.

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1278968101)
any reason to not just use the task name, like before?

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1278961684)
Can delete the arm one now?

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1278981444)
> So if my understanding of the API is correct, you receive a package from AcceptPackage() ...

Correct

> we have asserted than all packages component linearize are either spending a UTXO in the chainstate set, or spending an in-package component, so the worst-case scenario from a DoS viewpoint will be the limits as set in BIP331 / nversion=3 ?

The first check in `AcceptPackage` is `IsPackageWellFormed` which requires the package to be 25 transactions at most. The linearization with `Min
...

👍 dergoegge approved a pull request: "fuzz: use `ConnmanTestMsg` in `connman`"
(https://github.com/bitcoin/bitcoin/pull/28091#pullrequestreview-1554266221)
utACK ecfe507e07e9bdab210e04ebd3fc3b8ae9d6a094

💬 dergoegge commented on pull request "test: Add UBSan `-fsanitize=integer` suppressions for `src/secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/28131#issuecomment-1657954674)
Concept ACK

💬 MarcoFalke commented on pull request "test: Add UBSan `-fsanitize=integer` suppressions for `src/secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/28131#issuecomment-1657964767)
lgtm ACK a7477744c5e1df56d3a1e9ab9fc400bfb0ef6ec3

💬 fanquake commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1657966963)
~0 Is there any reason we can't use the public beta of the arm64 runners? Have we tested that at-least?

💬 naumenkogs commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1279003069)
I was pointing out a specific (probably stronger-than-rest) benefit of randomization. So that if one wants to drop the randomization from the code, they consider that.

Feel free to ignore it.

💬 naumenkogs commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1657983184)
ACK 1e65aae8068ecf313a7c3b176dfc1326b3b67fbe

👍 MarcoFalke approved a pull request: "ci: Run "macOS native x86_64" job on GitHub Actions"
(https://github.com/bitcoin/bitcoin/pull/28187#pullrequestreview-1554323041)
> ~0 Is there any reason we can't use the public beta of the arm64 runners? Have we tested that at-least?

Can you add a link to documentation on how to set that up? With details on the runners, etc. In any case, I don't think it matters, unless you can point to a bug that was found with arm64, but not with amd64. In practice, I expect all bugs found by CI to be found by both macOS arches.

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1279021074)
Or just remove the arch from the filename? The included `export HOST` should already take care of it and avoid a filename ping-ping.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1657986362)
Dropped `DIR_IWYU`.
Updated logging tests with the above suggestion.

👋 fanquake's pull request is ready for review: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296)