💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466966956)
In commit "sqlite: guard against dangling to-be-reverted db transactions" (508fda2fddfef0cefedde9777c914aec72adf7fb)

I found this comment confusing because I couldn't figure out when failing to abort a transaction would ever be expected to happen, and what the error handling strategy was supposed to be when it did happen. Would maybe suggest: "// If transaction cannot be aborted, it means there is a bug or there has been data corruption. Try to recover in this case by closing and reopening th
...

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466968974)
In commit "test: sqlite, add coverage for dangling to-be-reverted db txns" (d4ebac8381801d024b69d5f1f5a13246c61edbe4)

I think it would be a little more consistent with other code to call this variable "batch" instead of "handler." Also calling it handler gets a little confusing when code below sets a handler on the handler.

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466956009)
In commit "sqlite: introduce HasActiveTxn method" (5670a21f5ef28748b6b7242c03b82f39dcbc983f)

Would be good to drop the parenthetical about autocommit mode here, since that's now an implementation detail of HasActiveTxn()

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466953925)
In commit "sqlite: introduce HasActiveTxn method" (5670a21f5ef28748b6b7242c03b82f39dcbc983f)

There appears to be a bug here. Previously it would return false if `m_db` is null. Now it will try to execute begin transaction. I think you need to keep the `!m_database.m_db` condition

👍 theStack approved a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1844474487)
Code-review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3

Went through another review round and only found some minor nits, mostly naming suggestions. If you retouch (or follow-up material):
* should use multi-line imports (see style guidelines in `test/functional/README.md`)
* commit 382894c3acd2dbf3e4198814f547c75b6fb17706 has a leading space in the commit subject

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466950294)
nit: the name `self.peer` doesn't say a lot, maybe something like `self.key_material` would be more descriptive? (though it's strictly speaking more than that, as it contains also stuff derived from the keys like garbage terminators and session id)

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466958424)
nit: could name this method in a way that it's more obvious that this is called in the asyncio callback context, e.g. `_on_data_v2_handshake`.

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466826427)
nit: not a big deal, but I feel this message would fit more to the call-site, when the garbage is actually passed to the transport layer (before `.send_raw_message()`).

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466979708)
nit: a bit shorter
```suggestion
return self.is_connected and self.last_message.get('version') and not self.supports_v2_p2p
```

📝 achow101 opened a pull request: "consensus: Store transaction nVersion as uin32_t"
(https://github.com/bitcoin/bitcoin/pull/29325)
Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.

Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid
...

💬 ariard commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1911072717)
> Yes, I'm analyzing this with the changes related to v3 in mind, in which we will remove the CSV 1.

I don’t know if it’s robust to remove the CSV 1 and potential interactions of invalidated chain of transactions with `m_recent_rejects`.

> But Alice doesn't need the preimage to spend them, this is a revoked commitment, she spends through the revocation path. Unless you're thinking of a different scenario this time?

Still the same scenario than laid out in my comment here: https://githu
...

💬 achow101 commented on pull request "refactor: Compile unreachable walletdb code":
(https://github.com/bitcoin/bitcoin/pull/29315#issuecomment-1911073205)
ACK fa3373d3adbace7e4665cf391363319a55a09a96

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1467012383)
This is gone now.

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1467012789)
Reverted thank you.

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1467014047)
Done, `CFeeRate` is the right format since we are representing fee rate.

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1467017299)
>Shouldn't there be a check in CreateTransactionInternal that makes sure we don't make transactions above this feerate, and an arg passed to BroadcastTransaction? I also don't see a test case for it.

Agreed

I Added a check in `CreateTransactionInternal` in 0c9e0c5b2218c154b62275794b878af7e1457647.

No need to pass an arg to `BroadcastTransaction` if the transaction fee rate is above `maxfeerate` it will not read `BroadcastTransaction` so it will just be a dead code.

I added a test fo
...

💬 ariard commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911085188)
i think there is @petertodd ’s https://petertodd.org/2024/one-shot-replace-by-fee-rate to weigh as a pinning solution.
sounds to me slightly more robust than v3 policy as no malleability in the fee-bumping mechanism.
however the dynamic N replace-by-feerate window might be a mess for miners mempools.
whatever the solution (v3 or replace-by-feerate), i believe you will still have exploitable asymmetries for L2s.

🚀 achow101 merged a pull request: "refactor: Compile unreachable walletdb code"
(https://github.com/bitcoin/bitcoin/pull/29315)

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1911091517)
Forced pushed from https://github.com/bitcoin/bitcoin/commit/2576efcf689401dcbea85856c74efc64af0c2cb2 to https://github.com/bitcoin/bitcoin/commit/0c9e0c5b2218c154b62275794b878af7e1457647 Compare the [diff](https://github.com/bitcoin/bitcoin/compare/2576efcf689401dcbea85856c74efc64af0c2cb2..0c9e0c5b2218c154b62275794b878af7e1457647)
- All review comments are addressed
- Removed `maxburnamount` option
- Made `maxfeerate` wallet option
- Added a check in `CreateTransactionInternal` that makes s
...

👋 ismaelsadeeq's pull request is ready for review: "Wallet: Add `maxfeerate` wallet startup option"
(https://github.com/bitcoin/bitcoin/pull/29278)