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

💬 yancyribbens commented on pull request "test: Add algo assert to bnb_search_test":
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1881523070)
hmm it looks like now it is testing `bnb` for all cases and not any are `knapsack`. Maybe my branch was stale and after re-basing it is now correct.

💬 TheCharlatan commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1881533617)
Re https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1880843285

> 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 see, that is a bit unfortunate. I also found some more things to change: https://github.com/TheCharlatan/bitcoin/commit/63fcfbecd133551032857187e5e639d36a2f7b06,
...

💬 yancyribbens commented on pull request "test: Add algo assert to bnb_search_test":
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1881542030)
Ah wait I see what happened. If the check for fee_rate is removed from `bnb` like I pointed out here: https://github.com/bitcoin/bitcoin/pull/26061/files there is test failure. However, the reason there's no test failure when that is removed is not that there is no tests, because there is here https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L403. However, if that is removed, the test still passes because it falls back to using knapsack. Anyway, by adding
...

💬 ryanofsky commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `void(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-1881545693)
> However we've generally held to the policy of not deleting any records from the wallet since that could result in private keys being accidentally deleted. The only exception to that was adding encryption.

Oh, I didn't know that but it makes sense.

One approach you could take would just be to delete the descriptor from memory without actually deleting it from the database, and ignore it the next time the wallet is loaded. But a drawback of this would be that once `addhdkey` was used the w
...

🤔 mzumsande reviewed a pull request: "p2p: Fill reconciliation sets (Erlay)"
(https://github.com/bitcoin/bitcoin/pull/28765#pullrequestreview-1809692459)
> Finally submitted the optimized code.

Did you perhaps push the wrong branch? You upvoted several of sipa's suggestions but they don't seem to be addressed in the current code.

💬 mzumsande commented on issue "new crash in v26.0":
(https://github.com/bitcoin/bitcoin/issues/29153#issuecomment-1881565460)
Looks like the crash happens within `libQt5Core.so.5`, for which there are no debug symbols available.

I tried for a while to reproduce the crash but didn't manage. Is there something special or "unusual" about your setup, for example are you loading a large number of wallets, or are some of the wallets very large?