💬 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)

💬 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

💬 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
...

💬 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
...

💬 theStack commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911138365)
Concept ACK

Another instance in the tests which could be adapted to 0xffffffff:
https://github.com/bitcoin/bitcoin/blob/717103bccec8d271f61f9cd6481b334bd9889146/src/test/transaction_tests.cpp#L783
Slightly related to this PR: since this currently doesn't produce a warning, was it ever considered to enable `-Wsign-conversion`?

💬 sipa commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911141799)
Concept ACK on making transaction's `nVersion` unsigned, as that is in practice how it already behaves.

Would it make sense to also rename the field to e.g. `unsigned_version` or so, so that:
* Reviewing the diff makes it obvious whether any references to the field exist that aren't modified.
* Any future contributor who missed this change won't accidentally assume the field still has signed semantics?

For normal code I'd consider this measure overkill, but this is pretty fundamental con
...

🤔 mzumsande reviewed a pull request: "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses"
(https://github.com/bitcoin/bitcoin/pull/26859#pullrequestreview-1844867102)
ACK b851c5385d0a0acec4493be1561cea285065d5dc

I let the `banman` fuzzer run for a while, and it didn't crash anymore.