💬 m3dwards commented on issue "ci: failure in `wallet_assumeutxo.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29234#issuecomment-1889549162)
Trying to replicate this locally and at the moment I get a different failure:

```shell
assert_raises_rpc_error(-4, "Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height 299", n1.restorewallet, "w", "backup_w.dat")
```

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

Tangential, I don't understand the macro ordering in this line:

```
The macros `LogInfo`, `LogDebug`, `LogTrace`, `LogWarning` and `LogError` are available for
```

🤔 ryanofsky reviewed a pull request: "RPC/Wallet: Convert walletprocesspsbt to use options parameter"
(https://github.com/bitcoin/bitcoin/pull/24963#pullrequestreview-1818316586)
Code review f43f992b7318086859f710b920b4427e6c657fe8. I think this looks pretty close, and the approach taken in this PR to preserves backwards compatibility seems pretty clean. I would have thought prior to this PR that we might need to break backwards compatibility to convert existing named parameters to options, but this is not the case. Two things I think should be improved:

1. Lack of enforcement for m_type_per_name vector. Right now the MatchesType implementation just checks if the para
...

💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1450514731)
In commit "RPC: Support specifying different types for param aliases" (c2612273ef0b3ef127ba03c87f2a3303f0b9d299)

I'm pretty sure you can simplify this and revert all changes to this function. All this is doing is getting the `sign` parameter type from the outer parameter declaration instead of the inner option declaration, which doesn't change anything because both types are boolean. The output of "bitcoin-cli -regtest help dump_all_command_conversions" is the same with or without this change
...

💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1450616033)
In commit "RPC: Support specifying different types for param aliases" (c2612273ef0b3ef127ba03c87f2a3303f0b9d299)

I don't think this check is strict enough, because it isn't actually verifying the parameter was passed with the corresponding name for the `m_type_per_name` value. It will accept a parameter with any type found in the vector, not just a parameter with the correct type for its name. This is not good because it means in the walletprocesspsbt case you could pass options=true and it w
...

👍 BrandonOdiwuor approved a pull request: "test: unbreak: exclude windows from wallet_assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/29238#pullrequestreview-1818617575)
Code Review ACK 99399ae88fa23c0333bc2b0d59b48d0936a869c7

💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1450676078)
just want to add that if this turned out annoying to fix (I didn't see an immediate trivial fix but haven't looked hard), I'd be fine just throwing an assert / error if decoy packages are received in this PR (since bitcoind shouldn't send those anyway).

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

I think it's not very clean to reach into `self.v2_state`'s internals to figure out how much `respond_v2_handshake` has consumed. It would be better if the function just returned how much was consumed (if anything).

💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1448904297)
In "In commit "[test] Construct class to handle v2 P2P protocol functions"

Nit: see above, I think it would be cleaner to return `(0, None)` in case no complete packet is present, and `(-1, None)` in case of error. That would make the description:
* int: number of bytes consumed (or -1 if error)
* bytes: contents of decrypted non-decoy packet if any (or None otherwise)

Since the callers don't care about the distinction between "no packet processed" or "decoy packet processed" for what to
...

💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1448895458)
In commit "[test] Construct class to handle v2 P2P protocol functions"

This needs to be `return processed_length, True`, as there may be been decoys before (I can trigger a failure in p2p_v2_encrypted.py by making Bitcoin Core send large decoys before the version packet).

I think it would be even cleaner to change `v2_receive_packet` to also return `contents = None` in case no complete packet was received. In that case, the whole `elif length == 0:` branch can go away.

💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1448911572)
In commit "[test] Introduce EncryptedP2PState object in P2PConnection":

Nit: probably unnecessary to go into the detail of what gets sent, as that's the responsibility of `v2_p2p`. Maybe just say "The initial handshake is sent immediately".

💬 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