💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1450663849)
In commit "[test] Perform initial v2 handshake":

I don't think this conditional is necessary. `authenticate_handshake` itself can figure out what it needs.

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1889620076)
> That would mean adding logic / additional options for it in possibly multiple spots, and having yet another place where net permissions are defined. If that becomes too messy, another option would be to leave it in -whitelist but just not apply it to automatic outbound connections.

I agree. I think the easiest path would be not applying these permissions for the automatic outbound connections, leaving it for the manual ones.

💬 m3dwards commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889628006)
I'm struggling to exactly replicate this locally. I get a failure and on the same line but my error seems to be the assert failing that expects an exception:

```
2024-01-12T16:34:39.449000Z TestFramework (INFO): Backup can't be loaded during background sync
2024-01-12T16:34:39.496000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "C:\Users\Max\source\bitcoin\test\functional\test_framework\util.py", line 140, in try_rpc
fun(*args, **kwds)
File "C
...

💬 achow101 commented on issue "ci: failure in `wallet_assumeutxo.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29234#issuecomment-1889634306)
> Could this be a bug in the wallet?

Indeed, this looks more like a windows related bug when there's a wallet restoring failure.

> Why would it call `remove_all` on the wallet file of a completely different node?

I don't think it is. I think the problem is more along the lines of the same bitcoind has the database open at the time it tries to cleanup the failed restore.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1889635234)
I pulled #29032 into this branch to remove the need for branch-switching while testing on a custom signet.

💬 kristapsk commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1889643297)
Concept ACK

💬 achow101 commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889645425)
I don't think this fixes the problem.

💬 Eunovo commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1889653618)
I tested this locally and I've observed that the test doesn't fail if you modify the external wallet's timelocks, see below:
`external = multisig.getdescriptorinfo(f"wsh(thresh({self.N},pk({f'),s:pk('.join(external_xpubs)}),sln:after(128)))")`
The reason is that the test only sends from the external wallet once using all keys and the transactions that use the `locktimes` send from the internal wallet because that's where the funds are. Hence, changing the timelocks for the external descriptor
...

👍 kristapsk approved a pull request: "rpc: Make v2transport default for addnode RPC when enabled"
(https://github.com/bitcoin/bitcoin/pull/29239#pullrequestreview-1818719538)
ACK 3ba815b42db74804e341ce15f648c2b297af55ca

👍 theStack approved a pull request: "rpc: Make v2transport default for addnode RPC when enabled"
(https://github.com/bitcoin/bitcoin/pull/29239#pullrequestreview-1818732808)
Code-review ACK 3ba815b42db74804e341ce15f648c2b297af55ca

💬 jamesob commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889725982)
I've reverted to the dumb fix that will unbreak CI (but still add @Sjors' useful coverage for non-Windows platforms). If anyone can come up with something smarter that actually works, I'll be happy to review.

📝 instagibbs opened a pull request: "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2"
(https://github.com/bitcoin/bitcoin/pull/29242)
This is a smaller piece of https://github.com/bitcoin/bitcoin/pull/28984 broken off for easier review.

Up to date explanation of diagram checks are here: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393

This infrastructure has two near term applications prior to cluster mempool:
1) Limited Package RBF(https://github.com/bitcoin/bitcoin/pull/28984): We want to allow package RBF only when we know it improves the mempool. This narrowly scoped functionality allows
...

💬 alfonsoromanz commented on pull request "doc: Add missing backtick in developer notes logging section":
(https://github.com/bitcoin/bitcoin/pull/29241#issuecomment-1889740992)
ACK c003562120c193c296ac754b4bb8cff02bbbe4dc

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-1889742216)
broke off all the incentive compatibility check stuff into its own PR: https://github.com/bitcoin/bitcoin/pull/29242

Putting this in draft for now to divert attention there

📝 instagibbs converted_to_draft a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984)
Allows any 2 transaction package with no in-mempool ancestors to do package RBF when directly conflicting with other mempool clusters of size two or less.


Proposed validation steps:
1) If the transaction package is of size 1, legacy rbf rules apply.
2) Otherwise the transaction package consists of a (parent, child) pair with no other in-mempool ancestors (or descendants, obviously), so it is also going to create a cluster of size 2). If larger, fail.
3) The package rbf may not evict more
...

✅ brunoerg closed a pull request: "rpc, p2p: add `addpermissionflags` RPC and allow whitelisting outbound"
(https://github.com/bitcoin/bitcoin/pull/26441)

💬 brunoerg commented on pull request "rpc, p2p: add `addpermissionflags` RPC and allow whitelisting outbound":
(https://github.com/bitcoin/bitcoin/pull/26441#issuecomment-1889742666)
Closing it to focus on other approach.

🤔 jonatack reviewed a pull request: "rpc: Make v2transport default for addnode RPC when enabled"
(https://github.com/bitcoin/bitcoin/pull/29239#pullrequestreview-1818764745)
Approach ACK

💬 jonatack commented on pull request "rpc: Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1450757056)
Test coverage for this conditional logic and error? Haven't looked deeply but didn't see it in https://github.com/bitcoin/bitcoin/pull/24748 @stratospher.

💬 jonatack commented on pull request "rpc: Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1450777060)
Perhaps good to disambiguate the config option from the rpc option.
```suggestion
{"v2transport", RPCArg::Type::BOOL, RPCArg::DefaultHint{"set by -v2transport configuration option"}, "Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command)"},
```