💬 mzumsande commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2096229309)
I don't understand this comment. Did you use prioritisetransaction in an older version, but dropped that?
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2096229309)
I don't understand this comment. Did you use prioritisetransaction in an older version, but dropped that?
🚀 ryanofsky merged a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221)
(https://github.com/bitcoin/bitcoin/pull/30221)
✅ ryanofsky closed an issue: "wallet: wrong balance and crash after reorg and unclean shutdown"
(https://github.com/bitcoin/bitcoin/issues/31824)
(https://github.com/bitcoin/bitcoin/issues/31824)
💬 ryanofsky commented on pull request "script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2892128673)
Might also help to add all the benchmark and test changes and simpler improvements like inlining in a separate PR, so it's a more clear how tricky the sensitive changes here are, and if the sensitive changes are really worth making.
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2892128673)
Might also help to add all the benchmark and test changes and simpler improvements like inlining in a separate PR, so it's a more clear how tricky the sensitive changes here are, and if the sensitive changes are really worth making.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096403730)
I understand the slight additional complexity concern but for a transaction submitted to us with no PoW i think we should discard it as soon as we know we won't accept it. I really don't think we should keep doing unnecessary work by iterating, solving scripts and counting.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096403730)
I understand the slight additional complexity concern but for a transaction submitted to us with no PoW i think we should discard it as soon as we know we won't accept it. I really don't think we should keep doing unnecessary work by iterating, solving scripts and counting.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096405194)
Don't you think it would be confusing to point here since this function does not concern itself with the witness? Especially as it's pointed right before.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096405194)
Don't you think it would be confusing to point here since this function does not concern itself with the witness? Especially as it's pointed right before.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096415934)
Done, added a sentence to this effect.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096415934)
Done, added a sentence to this effect.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096416043)
Done.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096416043)
Done.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096416080)
That's actually what i had initially but i reduced it to `MAX_LEGACY_SIGOPS` in an attempt to avoid too long variable names. With the comment expliciting it's about transaction inputs, i think it should be fine as-is.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096416080)
That's actually what i had initially but i reduced it to `MAX_LEGACY_SIGOPS` in an attempt to avoid too long variable names. With the comment expliciting it's about transaction inputs, i think it should be fine as-is.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096416254)
Expanded the test to check the original, non-standard, one can still be mined.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2096416254)
Expanded the test to check the original, non-standard, one can still be mined.
💬 darosior commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2096422126)
If it's set to the maximum RAM capacity on 32-bit system the process will start and then just randomly crash after having received enough blocks. 1 GiB seems like a reasonable maximum for a 32-bit system.
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2096422126)
If it's set to the maximum RAM capacity on 32-bit system the process will start and then just randomly crash after having received enough blocks. 1 GiB seems like a reasonable maximum for a 32-bit system.
💬 fjahr commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#issuecomment-2892174264)
ACK 7193245cd66791216d4e586ca09302b26d4b7528
Confirmed there are no such comments left outside of subtrees. Also seeing different full include lists suggested by IWYU but those seem exaggerated for this change here. Checked a hand full of the files that had bigger changes and didn't find anything wrong.
(https://github.com/bitcoin/bitcoin/pull/32562#issuecomment-2892174264)
ACK 7193245cd66791216d4e586ca09302b26d4b7528
Confirmed there are no such comments left outside of subtrees. Also seeing different full include lists suggested by IWYU but those seem exaggerated for this change here. Checked a hand full of the files that had bigger changes and didn't find anything wrong.
🤔 ryanofsky reviewed a pull request: "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors"
(https://github.com/bitcoin/bitcoin/pull/32475#pullrequestreview-2851907313)
Concept ACK 218db39973736ac50e912bec440b1e512586d3b0
(https://github.com/bitcoin/bitcoin/pull/32475#pullrequestreview-2851907313)
Concept ACK 218db39973736ac50e912bec440b1e512586d3b0
💬 ryanofsky commented on pull request "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors":
(https://github.com/bitcoin/bitcoin/pull/32475#discussion_r2096430609)
In commit "wallet: Use util::Error throughout AddWalletDescriptor" (218db39973736ac50e912bec440b1e512586d3b0)
I think I understand the need to use std::reference_wrapper with util::Result. But it would seem good to replace `std::optional<std::reference_wrapper<DescriptorScriptPubKeyMan>>` here and other places with just `DescriptorScriptPubKeyMan*` for simplicity and avoid reference_wrapper except in the cases where it's really needed. It seems like doing this would make a lot of the test cha
...
(https://github.com/bitcoin/bitcoin/pull/32475#discussion_r2096430609)
In commit "wallet: Use util::Error throughout AddWalletDescriptor" (218db39973736ac50e912bec440b1e512586d3b0)
I think I understand the need to use std::reference_wrapper with util::Result. But it would seem good to replace `std::optional<std::reference_wrapper<DescriptorScriptPubKeyMan>>` here and other places with just `DescriptorScriptPubKeyMan*` for simplicity and avoid reference_wrapper except in the cases where it's really needed. It seems like doing this would make a lot of the test cha
...
🤔 instagibbs reviewed a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2840518650)
acb2b6a90a47567773268a024bcf2f1ddfcc60df
focused on motivation/structure/approach and test coverage for first pass. Haven't validated the actual trimming logic yet.
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2840518650)
acb2b6a90a47567773268a024bcf2f1ddfcc60df
focused on motivation/structure/approach and test coverage for first pass. Haven't validated the actual trimming logic yet.
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089137415)
462429bc159b1b5ddb5443c484c9bdeae985ad26
nitty: max_agg_size
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089137415)
462429bc159b1b5ddb5443c484c9bdeae985ad26
nitty: max_agg_size
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089151874)
462429bc159b1b5ddb5443c484c9bdeae985ad26
misinterpreted the commit message that the _aggregate_ size should not be hit when adding transaction, rather than each individual txn shouldn't exceed this limit. Maybe make this crystal clear.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089151874)
462429bc159b1b5ddb5443c484c9bdeae985ad26
misinterpreted the commit message that the _aggregate_ size should not be hit when adding transaction, rather than each individual txn shouldn't exceed this limit. Maybe make this crystal clear.
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089169224)
462429bc159b1b5ddb5443c484c9bdeae985ad26
nit: docstring for `m_group_oversized` should be updated
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089169224)
462429bc159b1b5ddb5443c484c9bdeae985ad26
nit: docstring for `m_group_oversized` should be updated
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089187012)
462429bc159b1b5ddb5443c484c9bdeae985ad26
Was a bit confusing at first, but this must be true because clusters are never merged (via dependency application) when to-be-merged-clusters would be oversized.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089187012)
462429bc159b1b5ddb5443c484c9bdeae985ad26
Was a bit confusing at first, but this must be true because clusters are never merged (via dependency application) when to-be-merged-clusters would be oversized.
💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089177610)
and can't also be 0? Didn't double check this was already the case, but an assertion is added in 462429bc159b1b5ddb5443c484c9bdeae985ad26
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2089177610)
and can't also be 0? Didn't double check this was already the case, but an assertion is added in 462429bc159b1b5ddb5443c484c9bdeae985ad26