👍 hebasto approved a pull request: "RFC: Deprecate libconsensus"
(https://github.com/bitcoin/bitcoin/pull/29189#pullrequestreview-1809338454)
ACK a6745f8be306a0aeeb7fb070297ba503321891d0, the diff is correct.

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1444825855)
`testmempoolaccept` is basically shoving all transactions, whether or not they're a pacakge, into `AcceptMultipleTransactions`.

I'm not sure it makes sense due to this.

💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#issuecomment-1881230813)
> Still working on this? I'll be available to re-review quickly after comments are addressed.

yes sorry I can try and make some followup changes to the comments soon

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1444836670)
Ignoring topo restrictions in `AcceptPackage` since `C` should have `A` as a direct ancestor,

> 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 subpackage, which might not be incentive compatible at the current fee estimates.

This is prior to any relay, of course restricting feerates is incentive compatible once published on the open network!

> If maxfeerate is 50s/vb we will
...

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1444841601)
startup config? Maybe? Could open an issue

💬 sipa commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1881252737)
Dropped the last commit, and improved the code changes a bit. For backport, if needed, the first commit suffices.

✅ achow101 closed a pull request: "Open fully encrypted wallets"
(https://github.com/bitcoin-core/gui/pull/747)

💬 maflcko commented on pull request "build: Drop `ALLOW_HOST_PACKAGES` support in depends":
(https://github.com/bitcoin/bitcoin/pull/29203#discussion_r1444886977)
```suggestion
export PACKAGES="gcc-10 g++-10 python3-zmq"
```

💬 hebasto commented on pull request "build: Drop `ALLOW_HOST_PACKAGES` support in depends":
(https://github.com/bitcoin/bitcoin/pull/29203#discussion_r1444892896)
Thanks! Fixed.

👋 achow101's pull request is ready for review: "Dialog for allowing the user to choose the change output when bumping a tx"
(https://github.com/bitcoin-core/gui/pull/700)

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1444942956)
added a mention of the default being 0

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1444943017)
taken

💬 jamesob commented on pull request "test: wallet migration, add coverage for tx extra data":
(https://github.com/bitcoin/bitcoin/pull/29204#issuecomment-1881425364)
Nice, ACK https://github.com/bitcoin/bitcoin/pull/29204/commits/016cc807f77a9128d430a0df1edd133521628a33

💬 jamesob commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1881435270)
Two ACKs on a test change - RFM?

📝 fanquake opened a pull request: "build: always set `-g -O2` in `CORE_CXXFLAGS`"
(https://github.com/bitcoin/bitcoin/pull/29205)
Rather than trying to sporadically rely on / override Autoconf default behaviour. Just always override, and always set the flags we want (which are the same as the Autoconf defaults).

Removes the need for duplicate code to clear (if not overridden) `CXXFLAGS`.

Fixes cases of "missing" `-O2`. i.e this PR when running a Valgrind CI job with changes here:
```bash
CXXFLAGS = -g -O2 -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -mbranch-protection=bt
...

💬 jamesob commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1443338475)
This doesn't merit any updating in non-test code because we never call `AddCoin()` within a `try` outside of tests, right?

💬 jamesob commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1445009004)
To confirm: the removal of `ConsumeUint256()` here is okay (and _doesn't_ cause this test to never rotate the best block) because of the `BatchWrite()` call below, right?

For `!is_db` cases this is now a no-op, whereas before it wasn't. Is that intended?

📝 yancyribbens opened a pull request: "test: Add algo assert to bnb_search_test"
(https://github.com/bitcoin/bitcoin/pull/29206)
Adds algo assertion to bnb_search tests. This also highlights a test bug [here](https://github.com/bitcoin/bitcoin/commit/bd8ddfaf1cddcd3d51f84ab396ce8273ed607396#diff-36ddaeb9e3a5c1aaaccd6b1ed6c770e8344e33dbfd4876b5f0726d84ab47cbabR433). The case where fees are high which should cause bnb to select fewer inputs isn't actually being tested here since the result is making assertion on what's returned by `knapsack`, not `bnb`.

💬 ryanofsky commented on pull request "init: handle empty settings file gracefully":
(https://github.com/bitcoin/bitcoin/pull/29144#issuecomment-1881485735)
re: 725a1fc7a7dbb1153bec188eda2f17a217560352 I think it is important to treat an empty settings file as an error, because the most likely cause of an empty file is some kind of crash or corruption or disk full state, and it is safer to alert the user about the situation so they can fix it, than proceed as if the error didn't happen and lose important settings that could affect security or stability.

I think the error message "Unable to parse settings file %s" could definitely be improved thou
...

💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1881518666)
Thank you for the review, and good idea @kevkevinpal!

> I think it would be cool if instead of always using the pubkeys in the same order when decaying we could randomly use different signers

I added a 2nd commit to this PR with your suggestion. In a similar spirit I improved #29154 (also adding a 2nd commit with your suggestion, and in that case we can also assert that order of xpubs in the multisig descriptors don't matter since it is `sorted`).

> Also, one thing I am unsure of is if
...