💬 Sjors commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1620356102)
Thanks for improving these tests. Added to review list.

💬 t-bast commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1620368501)
> I’ve fixed a number of issues, if this latest push’s checks turn up green, it might be more fruitful to try now.

I confirm that, I've ran my set of tests against [eclair](https://github.com/ACINQ/eclair) and everything looks good using https://github.com/bitcoin/bitcoin/pull/26152/commits/0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 :tada:

The tests I ran can be found in the last two commits of [this branch](https://github.com/ACINQ/eclair/commits/wip-test-ancestor-aware-funding-bitcoind-25
...

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1620375694)
> I don't think you need to special case individual transactions even, actually. Instead, use this, instead of chunking:

I'm not sure about using a group's aggregate feerate without checking their spending relationships, as it may allow unrelated transactions to pay for each other. For example:
```
A(1) B(3)
^ ^
C(100)
```
Where minfeerate is 2sat/vB. Imagine C is invalid (e.g. a fake child created to connect A and B).
The ancestor score-based linearization I was imagining (i
...

💬 instagibbs commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1252120078)
> download should be dedup when we receive the ancpkginfos ?

If it's in your orphanage it won't be fetched again. See `ReceivedAncPkgInfo`

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1620395050)
Nothing is perfect, but a linearizer would ideally pick `B` before `A`, yes. So you might get:

`B, A, C`
or
`B, C, A`

depending on if the strategy is greedy. "topo" sort may miss this, which is why we should probably be smarter than that?

👍 hebasto approved a pull request: "Remove confusing "Dust" label from coincontrol / sendcoins dialog"
(https://github.com/bitcoin-core/gui/pull/719#pullrequestreview-1513056670)
Looks good. ACK a582b4141f0756faa3793fb1c772898a984c83e4.

💬 sipa commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1620413203)
@glozow You're right; I didn't think this through.

@instagibbs To a limited extent better linearization can help here (though within-chunk optimization isn't something I've been looking at, as it doesn't matter for mining/eviction) but I think you can construct more complex examples where even a "perfect" linearization results in grouping of things that should not pay for each other.

I'm starting to think that something closer to your idea here is right: trying ancestor sets of every transacti
...

✅ hebasto closed an issue: "Confusing/misleading "Dust:" label in coin selection dialog"
(https://github.com/bitcoin-core/gui/issues/699)

🚀 hebasto merged a pull request: "Remove confusing "Dust" label from coincontrol / sendcoins dialog"
(https://github.com/bitcoin-core/gui/pull/719)

💬 luke-jr commented on pull request "build: pass sanitize flags to instrument libsecp256k1 code":
(https://github.com/bitcoin/bitcoin/pull/27991#discussion_r1252148488)
Won't this lose the other `CFLAGS` currently passed by the user?

💬 luke-jr commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-1620432636)
If both are provided, I would expect both to be active in parallel, and complete even if the other succeeds...

🤔 luke-jr requested changes to a pull request: "init: adding check for : for -torcontrol flag"
(https://github.com/bitcoin/bitcoin/pull/28018#pullrequestreview-1513100420)
Concept NACK. Port isn't required (the default is 9051). Furthermore, even "1" is a valid value (it would be the same as 0.0.0.1:9051), though I suppose it wouldn't hurt to check for it specifically and reject it.

💬 luke-jr commented on pull request "exclude ipc scheme from port check":
(https://github.com/bitcoin/bitcoin/pull/28020#discussion_r1252163957)
`rfind` is a very weird choice here. Suggest using [`compare`](https://cplusplus.com/reference/string/string/compare/)

💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1620448305)
Building 8587bfbbbc33f2b5e3737c9afe42e6d09464064c fails with:

```
wallet/wallet.cpp: In function ‘std::shared_ptr<wallet::CWallet> wallet::CreateWallet(WalletContext&, const std::string&, std::optional<bool>, DatabaseOptions&, DatabaseStatus&, bilingual_str&, std::vector<bilingual_str>&)’:
wallet/wallet.cpp:342:34: error: ‘WALLET_FLAG_GLOBAL_HD_KEY’ was not declared in this scope
342 | wallet_creation_flags |= WALLET_FLAG_GLOBAL_HD_KEY;
| ^
...

👍 luke-jr approved a pull request: "Switch RPCConsole wallet selection to the one most recently opened/restored/created"
(https://github.com/bitcoin-core/gui/pull/696#pullrequestreview-1513117159)
utACK 99c0eb9701e71f16aa360a420b7e4851d5b92510

💬 willcl-ark commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1620461536)
OK I've pushed a new set of changes which now disconnects nodes synchronously inside of `AttemptToEvictConnection`.

@Crypt-iQ I'd be curious if you still see these new changes as resolving the issue in #27843? I havent' gotten your test patch working to my satisfaction yet (or at least, I don't see positive eviction candidate selection during it so it wouldn't overflow `nMaxInbound` even without these changes).

👍 hebasto approved a pull request: "Switch RPCConsole wallet selection to the one most recently opened/restored/created"
(https://github.com/bitcoin-core/gui/pull/696#pullrequestreview-1513125185)
ACK 99c0eb9701e71f16aa360a420b7e4851d5b92510, tested on Ubuntu 23.04.

🚀 hebasto merged a pull request: "Switch RPCConsole wallet selection to the one most recently opened/restored/created"
(https://github.com/bitcoin-core/gui/pull/696)

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1620477978)
> Looks like a slightly non trivial rebase. For context, which PR's triggered it?

#24914 I think



> Building [8587bfb](https://github.com/bitcoin/bitcoin/commit/8587bfbbbc33f2b5e3737c9afe42e6d09464064c) on Ubuntu 23.04 with gcc 12.2.0 fails with:
>
> ```
> wallet/wallet.cpp: In function ‘std::shared_ptr<wallet::CWallet> wallet::CreateWallet(WalletContext&, const std::string&, std::optional<bool>, DatabaseOptions&, DatabaseStatus&, bilingual_str&, std::vector<bilingual_str>&)’:
> wa
...

💬 MarcoFalke commented on pull request "ci: Remove deprecated container.greedy":
(https://github.com/bitcoin/bitcoin/pull/28024#issuecomment-1620483829)
Dropped the CCACHE_SIZE stuff and restored the initial change