💬 fjahr commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2391539772)
nit: Not sure if assert is needed in both places now, seems like keeping it in `ComputeSchnorrSignatureHash` might be enough.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2391539772)
nit: Not sure if assert is needed in both places now, seems like keeping it in `ComputeSchnorrSignatureHash` might be enough.
💬 fjahr commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392889562)
Should this check that no value exists already for this key? Otherwise, if the key wasn't deleted properly and session id is reused somehow, there will be no effect and this might lead to danger reusing the nonce? It's a bit far fetched but might be good as belt and suspenders.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392889562)
Should this check that no value exists already for this key? Otherwise, if the key wasn't deleted properly and session id is reused somehow, there will be no effect and this might lead to danger reusing the nonce? It's a bit far fetched but might be good as belt and suspenders.
💬 fjahr commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2395048854)
nit: I wouldn't call a `continue` exit early, I would expect to exit the whole function here then.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2395048854)
nit: I wouldn't call a `continue` exit early, I would expect to exit the whole function here then.
🤔 glozow reviewed a pull request: "Mempool: Do not enforce TRUC checks on reorg"
(https://github.com/bitcoin/bitcoin/pull/33504#pullrequestreview-3281925415)
ACK 06df14ba75b
We don't re-check TRUC rules for descendants of reorged transactions because (1) there would be a performance hit and (2) we'd prefer to keep those fee-paying transactions: https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1409812453. I agree it makes sense to also skip the ones in blocks themselves for similar reasons, especially as there are miners that don't apply the TRUC rules - it'd be good to re-mine their transactions if there are reorgs.
(https://github.com/bitcoin/bitcoin/pull/33504#pullrequestreview-3281925415)
ACK 06df14ba75b
We don't re-check TRUC rules for descendants of reorged transactions because (1) there would be a performance hit and (2) we'd prefer to keep those fee-paying transactions: https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1409812453. I agree it makes sense to also skip the ones in blocks themselves for similar reasons, especially as there are miners that don't apply the TRUC rules - it'd be good to re-mine their transactions if there are reorgs.
💬 glozow commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2389364292)
While `PackageTRUCChecks` isn't in the reorg calling path, I think it would be cleaner and more future-proof to also gate that using `if (!args.m_bypass_limits)`. There are `Assume`s in that function relying on the fact that `SingleTRUCChecks` is called first.
If you'd prefer not to, I'd recommend a comment in the commit message explaining that PackageTRUCChecks are not gated here because reorgs do not ever go through `AcceptMultipleTransactions`.
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2389364292)
While `PackageTRUCChecks` isn't in the reorg calling path, I think it would be cleaner and more future-proof to also gate that using `if (!args.m_bypass_limits)`. There are `Assume`s in that function relying on the fact that `SingleTRUCChecks` is called first.
If you'd prefer not to, I'd recommend a comment in the commit message explaining that PackageTRUCChecks are not gated here because reorgs do not ever go through `AcceptMultipleTransactions`.
💬 glozow commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2392980542)
nit: documentation for `bypass_limits` in validation.h could mention that TRUC rules are not enforced and it's intended for reorgs
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2392980542)
nit: documentation for `bypass_limits` in validation.h could mention that TRUC rules are not enforced and it's intended for reorgs
💬 glozow commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2389596534)
Suggestion for a few more within-block test cases, if you're interested (can also just open a followup, have some other test ideas as well)
```
diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py
index 098631b5c41..58c69f07819 100755
--- a/test/functional/mempool_truc.py
+++ b/test/functional/mempool_truc.py
@@ -167,34 +167,76 @@ class MempoolTRUC(BitcoinTestFramework):
self.log.info("Test that, during a reorg, TRUC rules are not enforced")
...
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2389596534)
Suggestion for a few more within-block test cases, if you're interested (can also just open a followup, have some other test ideas as well)
```
diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py
index 098631b5c41..58c69f07819 100755
--- a/test/functional/mempool_truc.py
+++ b/test/functional/mempool_truc.py
@@ -167,34 +167,76 @@ class MempoolTRUC(BitcoinTestFramework):
self.log.info("Test that, during a reorg, TRUC rules are not enforced")
...
📝 theuni opened a pull request: "multiprocess: Fix high overhead from message logging"
(https://github.com/bitcoin/bitcoin/pull/33517)
This fixes https://github.com/bitcoin-core/libmultiprocess/issues/215 on Core's side. It depends on https://github.com/bitcoin-core/libmultiprocess/pull/220 being merged upstream, and a PR to update our subtree. I've included a subtree merge from my repo here for now, but will rebase on top of the merge from upstream once it's in.
For context: without https://github.com/bitcoin-core/libmultiprocess/pull/220, libmultiprocess serializes every log message parameter, even if that message was ulti
...
(https://github.com/bitcoin/bitcoin/pull/33517)
This fixes https://github.com/bitcoin-core/libmultiprocess/issues/215 on Core's side. It depends on https://github.com/bitcoin-core/libmultiprocess/pull/220 being merged upstream, and a PR to update our subtree. I've included a subtree merge from my repo here for now, but will rebase on top of the merge from upstream once it's in.
For context: without https://github.com/bitcoin-core/libmultiprocess/pull/220, libmultiprocess serializes every log message parameter, even if that message was ulti
...
💬 theuni commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#issuecomment-3358431806)
I don't understand why this is necessary. Why not use `sendmsgtopeer` if you're just sending dumb bytes that don't affect our state?
(https://github.com/bitcoin/bitcoin/pull/33507#issuecomment-3358431806)
I don't understand why this is necessary. Why not use `sendmsgtopeer` if you're just sending dumb bytes that don't affect our state?
💬 polespinasa commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#issuecomment-3358456391)
> I don't understand why this is necessary. Why not use `sendmsgtopeer` if you're just sending dumb bytes that don't affect our state?
We don't want to send invalid txs or txs that we would not broadcast,. We want to validates the txs before sending it to a peer.
Additionally, `peer_id` argument could take an array of ids and send the tx to multiple peers. (this is not implemented, but shouldn't be difficult to)
(https://github.com/bitcoin/bitcoin/pull/33507#issuecomment-3358456391)
> I don't understand why this is necessary. Why not use `sendmsgtopeer` if you're just sending dumb bytes that don't affect our state?
We don't want to send invalid txs or txs that we would not broadcast,. We want to validates the txs before sending it to a peer.
Additionally, `peer_id` argument could take an array of ids and send the tx to multiple peers. (this is not implemented, but shouldn't be difficult to)
💬 VegArchie commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3358462415)
@Dorex45
I want progress for Bitcoin as well. I don't think these OPRETURN changes are the best way to do it. If this is the only solution, then I feel the risks don't outweigh the rewards.
That being said, wouldn't it be worth sacrificing a little more time for Bitcoin's scaling progress, in order to look into alternate methods a little more? Isn't it worth quelling the out-of-control, online 'debates,' in the name of Bitcoin security?
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3358462415)
@Dorex45
I want progress for Bitcoin as well. I don't think these OPRETURN changes are the best way to do it. If this is the only solution, then I feel the risks don't outweigh the rewards.
That being said, wouldn't it be worth sacrificing a little more time for Bitcoin's scaling progress, in order to look into alternate methods a little more? Isn't it worth quelling the out-of-control, online 'debates,' in the name of Bitcoin security?
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2396179351)
Having some partial sigs and not all pubnonces should be a contradiction. It is not possible to create a valid partial sig without all of the pubnonces. I think in that situation it is safer to do nothing rather than try to continue by adding a new pubnonce.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2396179351)
Having some partial sigs and not all pubnonces should be a contradiction. It is not possible to create a valid partial sig without all of the pubnonces. I think in that situation it is safer to do nothing rather than try to continue by adding a new pubnonce.
💬 marcofleon commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2396228628)
Shouldn't `*block` be passed in here instead of `header`? Right now, I think the block just ends up with the random nonce from when the header was created. Although this still works half the time with the simplifed PoW check. Also, calling `GetHash()` on the header includes the merkle root, so finalizing the header should be after we calculate the merkle root below.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2396228628)
Shouldn't `*block` be passed in here instead of `header`? Right now, I think the block just ends up with the random nonce from when the header was created. Although this still works half the time with the simplifed PoW check. Also, calling `GetHash()` on the header includes the merkle root, so finalizing the header should be after we calculate the merkle root below.
💬 Geremia commented on issue "No way to clear command history in RPC console or reset the console without restarting the node":
(https://github.com/bitcoin-core/gui/issues/897#issuecomment-3358599699)
Restarting `bitcoin-qt` doesn't clear the command history for me.
(https://github.com/bitcoin-core/gui/issues/897#issuecomment-3358599699)
Restarting `bitcoin-qt` doesn't clear the command history for me.
💬 ajtowns commented on issue "[29.x] guix build failure on ppc with xcb":
(https://github.com/bitcoin/bitcoin/issues/33488#issuecomment-3358617644)
Running guix-build from a mostly clean build directly three times gives me these logs, if that helps:
https://gist.github.com/ajtowns/e670603ba85bb7720cea8fc5e1215172
Maybe my guix profile is corrupted or otherwise wrong somehow?
(https://github.com/bitcoin/bitcoin/issues/33488#issuecomment-3358617644)
Running guix-build from a mostly clean build directly three times gives me these logs, if that helps:
https://gist.github.com/ajtowns/e670603ba85bb7720cea8fc5e1215172
Maybe my guix profile is corrupted or otherwise wrong somehow?
💬 kingbillie95-max commented on something "":
(https://github.com/bitcoin/bitcoin/commit/5b8752198e979ea4987d8b6238f42f8ed9a38846#commitcomment-166969360)
5b8752198e979ea4987d8b6238f42f8ed9a38846
(https://github.com/bitcoin/bitcoin/commit/5b8752198e979ea4987d8b6238f42f8ed9a38846#commitcomment-166969360)
5b8752198e979ea4987d8b6238f42f8ed9a38846
💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2396429458)
Why was this line moved?
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2396429458)
Why was this line moved?
💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2396433735)
Should we also throw here if m_dirty_count is not zero?
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2396433735)
Should we also throw here if m_dirty_count is not zero?
💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2396423379)
The benefit of the change in this commit is unclear. I think it could just be removed to make review easier.
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2396423379)
The benefit of the change in this commit is unclear. I think it could just be removed to make review easier.
💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2396411224)
> Adds test coverage by randomly calling `EmplaceCoinInternalDANGER` in `SimulationTest` to verify the accounting remains correct.
Where do we verify that the accounting is correct? This also checks that the map does not contain this coin, so the `if (inserted) {` block will always be true and we never test the other case.
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2396411224)
> Adds test coverage by randomly calling `EmplaceCoinInternalDANGER` in `SimulationTest` to verify the accounting remains correct.
Where do we verify that the accounting is correct? This also checks that the map does not contain this coin, so the `if (inserted) {` block will always be true and we never test the other case.