💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2248646240)
```suggestion
// If this tx has a parent, unset its truc_child_in_mempool to make it possible
// to spend from the parent again. If this tx was replaced by another
```

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2248686896)
This comment can be confusing - we're not actually looking for transactions spending from *this* tx. We're looking for siblings of this tx, which spend from utxos of `wtx` (which is actually this transaction's parent). Might be helpful to rename `wallet_it` to `parent_it` and update this comment.

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2248640723)
1910d9b61f23f65335367f2f8dd021ac1ccd907a

Could be more specific. Also, this isn't only used by AvailableCoinsListUnspent. It's always checked in `AvailableCoins`, but `AvailableCoinsListUnspent` is the only caller that disables it because it's not building a transaction.

```suggestion
// When true, filter unconfirmed coins by whether their version's TRUCness matches what is set by CCoinControl.
bool check_version_trucness{true};
```

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2248721755)
Now that there is `send` and `sendall` support, is it just transactions received by the wallet?

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2248694635)
9b478f1f68c00b5c514a30a53d19cd58fe813795

integers can just be passed by value

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2248729121)
Missing test coverage for this condition I think? Need `user_input_weight_not_overwritten` test for something spending unconfirmed v3

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2248722112)
Maybe mention TRUC and/or BIP 431?

📝 ishaanam opened a pull request: "test: fix anti-fee-sniping off-by-one error"
(https://github.com/bitcoin/bitcoin/pull/33118)
This fixes the off-by-one error in the anti-fee-sniping tests for `send` and `sendall`. `assert_greater_than` fails if the two values are equal.

Closes #33114

💬 glozow commented on issue "intermittent issue in wallet_sendall.py", line 440, in sendall_anti_fee_sniping assert_greater_than(tx_from_wallet["decoded"]["locktime"], tx_from_wallet["blockheight"]":
(https://github.com/bitcoin/bitcoin/issues/33114#issuecomment-3145590340)
#33118

💬 glozow commented on pull request "test: fix anti-fee-sniping off-by-one error":
(https://github.com/bitcoin/bitcoin/pull/33118#discussion_r2248749935)
Trailing whitespace

🤔 glozow reviewed a pull request: "test: fix anti-fee-sniping off-by-one error"
(https://github.com/bitcoin/bitcoin/pull/33118#pullrequestreview-3080386253)
You could also `assert_greater_than_or_equal`?

💬 ishaanam commented on pull request "test: fix anti-fee-sniping off-by-one error":
(https://github.com/bitcoin/bitcoin/pull/33118#discussion_r2248761466)
Fixed

💬 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_r2248763843)
I didn't use this patch, but I put some effort into making the code more readable.

💬 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_r2248764330)
Thanks for this. I've updated the comment.

💬 murchandamus commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2248767523)
Thanks for the clarification and adding it to the commit message.

💬 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#issuecomment-3145623139)
> Mostly happy with [d82dcf2](https://github.com/bitcoin/bitcoin/commit/d82dcf2eaa7efb3809ae559c8f97f74403a2ccd6), but in [b32c2b2](https://github.com/bitcoin/bitcoin/commit/b32c2b290f8e4dcb7fe54ee4c81714a3785920a7) _descriptor: ToPrivateString() pass if at least 1 priv key exists_ I find the miniscript change hard to follow (mostly because I find the miniscript code itself hard to follow). Can you split that out in a seperate commit that doesn't change behaviour?

I split the original commit
...

💬 l0rinc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3145625422)
Thanks, rebased to solve conflict in `validations.cpp` and migrated a few new values - thanks for the hint @janb84.
You can re-review via `git range-diff master 71911a59 53461dc5`

💬 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_r2248786530)
`has_priv_key` is only relevant when `ToPrivateString` is called on an actual Descriptor instance. IIUC, this test doesn't do that, so the value of `has_priv_key` does not matter here.
I'm not sure we should set it here, apart from the fact that it doesn't affect the test, the string returned doesn't have a private key.

💬 D33r-Gee commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3145748442)
> I plan to test this soon(tm).

Thanks @Sjors !

💬 pablomartin4btc commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2248869900)
We have the assert above so we don't need to check if it's a descriptor and legacy can't be created anymore.
```suggestion
walletInstance->SetupDescriptorScriptPubKeyMans();
```