💬 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
(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
(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)
(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`.
(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()`).
(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
```
(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
...
(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
...
(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
(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.
(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.
(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.
(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
...
(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.
(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)
(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
...
(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)
(https://github.com/bitcoin/bitcoin/pull/29278)
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1467027522)
I think this comes straight from BIP324 pseudocode: https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#keys-and-session-id-derivation
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1467027522)
I think this comes straight from BIP324 pseudocode: https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#keys-and-session-id-derivation
💬 ismaelsadeeq commented on pull request "doc: clarify `BroadcastTransaction` comment":
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1467041934)
I think `BroadcastTransaction can be called by RPC or by the wallet` is ambiguous in this case.
By which RPC?
`BroadcastTransaction` can be invoked by other components that will like to submit transactions to the mempool and relay them to peers, in addition to the wallet and RPC.
It's accessible through the node interface https://github.com/bitcoin/bitcoin/blob/717103bccec8d271f61f9cd6481b334bd9889146/src/node/interfaces.cpp#L341
If I understand the rationale behind this comment correc
...
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1467041934)
I think `BroadcastTransaction can be called by RPC or by the wallet` is ambiguous in this case.
By which RPC?
`BroadcastTransaction` can be invoked by other components that will like to submit transactions to the mempool and relay them to peers, in addition to the wallet and RPC.
It's accessible through the node interface https://github.com/bitcoin/bitcoin/blob/717103bccec8d271f61f9cd6481b334bd9889146/src/node/interfaces.cpp#L341
If I understand the rationale behind this comment correc
...
💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1911119751)
> I just don't understand what motivated this PR, because "failure during the WalletBatch destruction" seems like something I would never expect to happen unless someone modified the code or manually corrupted the data on disk. So I'm wondering did we accidentally cause a breakage like that during development of these PRs? Or did you just notice that the error handling in the destructor wasn't very good, and decide to fix it? Or can failures to roll back actually happen in more cases than it see
...
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1911119751)
> I just don't understand what motivated this PR, because "failure during the WalletBatch destruction" seems like something I would never expect to happen unless someone modified the code or manually corrupted the data on disk. So I'm wondering did we accidentally cause a breakage like that during development of these PRs? Or did you just notice that the error handling in the destructor wasn't very good, and decide to fix it? Or can failures to roll back actually happen in more cases than it see
...