💬 ariard commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3099081175)
> This is already possible today. Besides i don't see how this is an issue: an untrusted counterparty can always make your
> funding protocol fail by providing invalid or non-standard inputs, or even just stopping to respond for that matter.

I don't think we're disagreeing that this new `MAX_TX_LEGACY_SIGOPS` can be a source of denial-of-service for collaborative transaction construction. But the point is that policy limits are already encompassed by second-layers flow to _sanitize_ invalid
...

🤔 murchandamus reviewed a pull request: "wallet, rpc: add anti-fee-sniping to `send` and `sendall`"
(https://github.com/bitcoin/bitcoin/pull/28944#pullrequestreview-3039897102)
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f

Compared to my prior ACK via range_diff, and only found this minor change and some changes due to the rebase.

```diff
- can_anti_fee_snipe &= (tx_in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL || tx_in.nSequence == MAX_BIP125_RBF_SEQUENCE);
+ can_anti_fee_snipe = can_anti_fee_snipe && (tx_in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL || tx_in.nSequence == MAX_BIP125_RBF_SEQUENCE);
```

Tested that the new tests fail without the first com
...

📝 achow101 opened a pull request: "wallet: Set descriptor cache upgraded flag for migrated wallets"
(https://github.com/bitcoin/bitcoin/pull/33031)
#32597 set the descriptor cache upgraded flag for newly created wallets, but migrated wallets still did not have the flag set when they are migrated. For consistency, and to avoid an unnecessary upgrade, we should be setting this flag for migrated wallets.

The flag would end up being set anyways at the end of migration when the wallet is reloaded as it would perform the automatic upgrade at that time. However, this is unnecessary and we should just set it from the get go.

This PR also adds
...

📝 achow101 opened a pull request: "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase"
(https://github.com/bitcoin/bitcoin/pull/33032)
`MockableDatabase` was introduced for the tests to avoid tying non-database tests to a particular database type. However, since the only database type now is sqlite, and because the mockable behavior is no longer used by the tests, we can replace usage of the `MockabeDatabase` with a SQLite database that lives only in memory.

This is particularly useful for future work that has the wallet make use of SQLite's capabilities more, which are less conducive to having a separate mock database imple
...

📝 achow101 opened a pull request: "wallet, sqlite: Encapsulate SQLite statements in a RAII class"
(https://github.com/bitcoin/bitcoin/pull/33033)
SQLite statements are C objects which can be long lived, so having them be initialized in a constructor, and cleanup everything in a destructor, makes the sqlite code cleaner and easier to read. This also lets us have the various functions needed to interact with sqlite statements be member functions.

This will be particularly useful in future work which makes use of more complicated SQL statements.

📝 achow101 opened a pull request: "wallet: Store transactions in a separate sqlite table"
(https://github.com/bitcoin/bitcoin/pull/33034)
The wallet uses SQLite as a key-value store even though SQLite is a powerful relational database engine. This causes us numerous headaches due to the need to serialize multiple fields together, and since record read from the database during loading can come in any order.

Notably, for transactions, if we were able to load them in the transaction order assigned by the wallet, PRs like #27865 would be a bit simpler and easier to reason about.

This PR makes it possible for us to do that by sto
...

🤔 murchandamus reviewed a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3040093165)
Concept ACK

💬 murchandamus commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2220548538)
Following the prior comment, I think it would be good to have a test where you are trying to build a transaction for which the version is not set, that has both a pre-selected version 1|2 and a pre-selected version 3 input.

💬 murchandamus commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2220508773)
In "wallet: throw error at conflicting tx versions in pre-selected inputs" (cf712ab56e3256db5dfcf422860bdb3cf87cd03e):
Is `m_version` always set?
Otherwise, what would happen if `coin_control.m_version` is not set, and one preselected input is TRUC, but another one is not?

💬 murchandamus commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2220523528)
In "wallet: don't include unconfirmed v3 txs with children in available coins" (bff006b7ae7fa120fac4f2118e891d2c88d38f9a):

I find this comment hard to parse. Do you mean something along the lines of:

```suggestion
// this marks all outputs of any unconfirmed v3 parent transactions as unspendable
// if any of its outputs are spent by this transaction
```

📝 marshallshen opened a pull request: "test: functional test for incomplete PSBT with additional signatures required"
(https://github.com/bitcoin/bitcoin/pull/33035)
## Summary

This PR adds a new functional test `test_psbt_incomplete_needs_additional_signature` to verify wallet behavior when a PSBT requires additional signatures and cannot be completed.

## What's Added
- Test scenario: Creates a 2-of-3 multisig transaction with only 1 signature provided
- Behavior validation: Confirms the wallet correctly identifies the transaction as incomplete and refuses to finalize

This test ensures that Bitcoin Core's wallet properly handles the common scenar
...

💬 l0rinc commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3100621871)
reACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948

Only difference since last review was applying https://github.com/bitcoin/bitcoin/pull/32945#discussion_r2218430945

💬 ajtowns commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#discussion_r2221068746)
Done

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2221456230)
Nice idea. Added you as Co-author in the relevant commit.

💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2221476916)
> The silent payments wallet needs to scan entire blocks since it doesn't know ahead of time which scriptPubKeys to use in a GCS filter

This is correct. I was imagining a world where we can pre-compute the scriptPubKeys, but realised we have to iterate the blocks anyways to get the tweaks so its likely faster to just check directly. In the future, if the wallet has some sort of silent payments index, then I think fast rescans would be possible (and beneficial).

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2221504575)
I forgot about the guard I added in https://github.com/bitcoin/bitcoin/pull/32471/commits/4202dca524aeef13e09380804c483d7c5402092d

You are right in the sense that it's not possible for a watch-only to even get to `ToPrivateString()` through the rpc; we could assert `has_priv_key` here. However, the descriptor unit tests often call `ToPrivateString()` on watch-only descriptors; assert `has_priv_key` causes the tests to fail.

🤔 vasild reviewed a pull request: "net: Fix Discover() not running when using -bind=0.0.0.0:port"
(https://github.com/bitcoin/bitcoin/pull/32757#pullrequestreview-3041523676)
Almost ACK a9b7542b6ea1af770d93cbbf5bb55dae6841334f

> I encountered port conflicts because the test framework automatically adds an onion bind to node 2,

Hmm, I think it is not the test framework but `bitcoind` itself that adds the onion bind. To figure out how `bitcoind` is started, e.g. for `nodes[2]` I look at `/tmp/bitcoin_func_test_.../node2/regtest/debug.log` for the lines like:
```
...
Config file arg: [regtest] shrinkdebugfile="0"
Config file arg: [regtest] unsafesqlitesync="1
...

💬 vasild commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2221491082)
The testing framework is designed so that it can run parallel executions on the same machine. For example, running two separate tests independently at the same time. To avoid port collisions between such independent executions is has `p2p_port(n)` and `tor_port(n)` which allocate ports in a range which (hopefully) will not overlap with the range from another test that executes concurrently (e.g. in another terminal). This `+100` seems a odds with this, maybe it could cause collisions.

I would
...

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2221513192)
To add this assertion, we will have to remove those checks, and I haven't seen the advantage in doing that.

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2221561291)
Yes, it does. The ImportingNow object is destroyed at the end of the function.