💬 maflcko commented on pull request "ci: Switch native macOS CI job to Xcode 15.0":
(https://github.com/bitcoin/bitcoin/pull/29195#issuecomment-1880726651)
pull title should be "build: Fix -Xclang -internal-isystem option"? Otherwise lgtm

💬 hebasto commented on pull request "build: Fix `-Xclang -internal-isystem` option":
(https://github.com/bitcoin/bitcoin/pull/29195#issuecomment-1880728913)
> pull title should be "build: Fix -Xclang -internal-isystem option"? Otherwise lgtm

Done.

💬 fanquake commented on pull request "build: Fix `-Xclang -internal-isystem` option":
(https://github.com/bitcoin/bitcoin/pull/29195#issuecomment-1880736065)
> The same Xcode version is used for the release binaries.

Can probably drop this, or be more specific, because we don't use Xcode to build release binaries (only the SDK, which is versioned differently).

💬 fanquake commented on pull request "ci: Do not set inane value for `LD_LIBRARY_PATH`":
(https://github.com/bitcoin/bitcoin/pull/29196#issuecomment-1880740854)
> For example, in the native macOS CI job,

Unless there's a downside to having this set, I don't think it's worth the extra code / shellcheck exceptions. i.e in this example, ld64 will never read this ENV var?

💬 hebasto commented on pull request "build: Fix `-Xclang -internal-isystem` option":
(https://github.com/bitcoin/bitcoin/pull/29195#issuecomment-1880740932)
> > The same Xcode version is used for the release binaries.
>
> Can probably drop this, or be more specific, because we don't use Xcode to build release binaries (only the SDK, which is versioned differently).

Dropped.

🚀 glozow merged a pull request: "RPC/Blockchain: scanblocks: Accept named param for filter_false_positives"
(https://github.com/bitcoin/bitcoin/pull/29184)

💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1444456650)
Should enable passing `maxfeerate` and `maxburnamount` as a config variables since it is used by various RPC, downstream might prefer this?

💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1444397171)
Shoulbe we testi `maxfeerate` and `maxburnamount` for `testmempoolaccept` with a package also?

💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1444464488)
To turn off for maxfeerate you should pass 0 fee rate, you cant turn off the restriction of `maxburnamount`
nit: maybe reword to relax?
```suggestion
# Relax the restrictions and send it; parent gets through as own subpackage
```

💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1444454903)
My understanding of package submission is that a child with unconfirmed parents is bundled together as a package for incentive compatibility. Should this instead check the package fee rate against `maxfeerate`?

If I understand correctly, currently, we are checking the package transaction fee rate individually against `maxfeerate`. The child transactions might be greater than the `maxfeerate`, but as a package might not be. In this case, we might reject the child transactions and accept the su
...

💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1444461653)
We did not document this anywhere but the default behavior if `maxburnamount` is not passed it we are rejecting the package if there is any transactions with unspendable output greater than 0.
Maybe indicate it in the `RPCHelpMan` of `maxburnamount`?

---

Its also not indicated for `sendrawtransaction`.

🤔 BrandonOdiwuor reviewed a pull request: "RFC: Deprecate libconsensus"
(https://github.com/bitcoin/bitcoin/pull/29189#pullrequestreview-1808889544)
Concept ACK

💬 TheCharlatan commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1880837705)
Can the full diff be reproduced with `run-clang-tidy -fix`? If so, can you post the full commands you are using? I am getting a slightly different diff if I run it myself.

💬 aureleoules commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1880843285)
> Can the full diff be reproduced with run-clang-tidy -fix? If so, can you post the full commands you are using? I am getting a slightly different diff if I run it myself.

Sorry, my initial push did not pass the tidy check because it seems the `-fix` option does not fix code in macros (all the asserts, Assume, etc.) so I manually fixed these in a second push. This may be the reason why you are not getting the same diff as me.

I don't have the specific command I used on hand but I used the
...

💬 fanquake commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1880865337)
Pushed up the patches into [my PR](https://github.com/Homebrew/homebrew-core/pull/156745), it has fixed some of the test failures, but not all: https://github.com/Homebrew/homebrew-core/actions/runs/7446626418/job/20257419440?pr=156745#step:4:82.
```bash
==> /home/linuxbrew/.linuxbrew/Cellar/bitcoin/26.0/bin/test_bitcoin
Running 585 test cases...
test/serfloat_tests.cpp(39): error: in "serfloat_tests/double_serfloat_tests": check repr == i has failed [1 != 0]
test/serfloat_tests.cpp
...

💬 fanquake commented on issue "brew: serfloat_tests tests fail on Linux":
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1880865889)
> Again, it would be good to have steps to reproduce locally.

See https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1880865337.

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1880891111)
Sorry for the slow response. I was away buit am now back.

Thanks for the review, I agree that setting special bits is uninteresting for cookie files (I was really trying to keep the subfunctions generic in case we wanted to re-use them elsewhere). I've now removed ability to set special bits.

I think a little on switching to string-based settings, i.e. `-rpccookieperms=group`, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same per
...

🤔 glozow reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-1808799983)
Combed through the code a bit more. Generally approach ACK for "cluster 2 replace cluster 2" idea.

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444482970)
Looks correct. If you're willing to entertain a suggestion to make it more readable, I think the following logic is easier to follow and the strings less ambiguous. Since "desc count' and "anc count" usually mean inclusive of tx, it's confusing if it's different in this function.

```suggestion
// Ancestor and descendant counts are inclusive of the tx itself.
const auto ancestor_count{direct_conflict->GetCountWithAncestors()};
const auto descendant_count{direct_confl
...

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444457214)
Maybe add "Each entry must be part of a cluster with size <=2."