💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2395761545)
It seems https://github.com/bitcoin/bitcoin/pull/32602/files#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R312-R321 revealed that there's a silent conflict here, we need the [first commit](https://github.com/bitcoin/bitcoin/pull/32313/commits/85979e2015344f05aa60db52b4a76eeadef216ad) from https://github.com/bitcoin/bitcoin/pull/32313 for this to pass after rebase
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2395761545)
It seems https://github.com/bitcoin/bitcoin/pull/32602/files#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R312-R321 revealed that there's a silent conflict here, we need the [first commit](https://github.com/bitcoin/bitcoin/pull/32313/commits/85979e2015344f05aa60db52b4a76eeadef216ad) from https://github.com/bitcoin/bitcoin/pull/32313 for this to pass after rebase
💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3357991920)
Rebased and adjusted the commits to minimize conflict with https://github.com/bitcoin/bitcoin/pull/33512
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3357991920)
Rebased and adjusted the commits to minimize conflict with https://github.com/bitcoin/bitcoin/pull/33512
👍 TheCharlatan approved a pull request: "init: Fix Ctrl-C shutdown hangs during wait calls"
(https://github.com/bitcoin/bitcoin/pull/33511#pullrequestreview-3291091712)
ACK ef7be125ea92e182f087d2c8e3d5c605b8df31d9
This should be backported in my opinion.
(https://github.com/bitcoin/bitcoin/pull/33511#pullrequestreview-3291091712)
ACK ef7be125ea92e182f087d2c8e3d5c605b8df31d9
This should be backported in my opinion.
💬 stringintech commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2395906680)
This makes sense and if we end up doing it, it seems it also allows removing some logic from `FindNextBlocksToDownload()` and have this:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 9ab6fb53ed..423834118c 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1397,24 +1397,15 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co
return;
}
- // Bootstrap quickly by guessing a parent of our bes
...
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2395906680)
This makes sense and if we end up doing it, it seems it also allows removing some logic from `FindNextBlocksToDownload()` and have this:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 9ab6fb53ed..423834118c 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1397,24 +1397,15 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co
return;
}
- // Bootstrap quickly by guessing a parent of our bes
...
💬 achow101 commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-3358144836)
ACK 87e7f37918d42c28033e9f684db52f94eeed617b
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-3358144836)
ACK 87e7f37918d42c28033e9f684db52f94eeed617b
🚀 achow101 merged a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326)
(https://github.com/bitcoin/bitcoin/pull/32326)
📝 GabrielSchnei opened a pull request: "doc: Correct typo 'implementes' to 'implements'"
(https://github.com/bitcoin/bitcoin/pull/33516)
Fixes a typo in `src\crc32c\.ycm_extra_conf.py` where "implementes" was used instead of "implements". This change improves readability and ensures consistency in the documentation.
No functional changes or tests are required, as this is a trivial documentation fix.
(https://github.com/bitcoin/bitcoin/pull/33516)
Fixes a typo in `src\crc32c\.ycm_extra_conf.py` where "implementes" was used instead of "implements". This change improves readability and ensures consistency in the documentation.
No functional changes or tests are required, as this is a trivial documentation fix.
💬 fanquake commented on pull request "doc: Correct typo 'implementes' to 'implements'":
(https://github.com/bitcoin/bitcoin/pull/33516#issuecomment-3358206687)
Thanks,this needs to go upstream.
(https://github.com/bitcoin/bitcoin/pull/33516#issuecomment-3358206687)
Thanks,this needs to go upstream.
✅ fanquake closed a pull request: "doc: Correct typo 'implementes' to 'implements'"
(https://github.com/bitcoin/bitcoin/pull/33516)
(https://github.com/bitcoin/bitcoin/pull/33516)
💬 hebasto commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3358249438)
My Guix build:
```
aarch64
ff3ab842fd36d8c51e76fa87d80e460e3a26fca8641543f01be1b7b62eea99d8 guix-build-93a70a42d30f/output/aarch64-linux-gnu/SHA256SUMS.part
d8e022a8f8794df818cf682e75da7ca2f633e5954cd306cf14cd06f62bc3405d guix-build-93a70a42d30f/output/aarch64-linux-gnu/bitcoin-93a70a42d30f-aarch64-linux-gnu-debug.tar.gz
c7ade0382153220a35b2d59a04f4c6578e40957898c468e0040555c49508039d guix-build-93a70a42d30f/output/aarch64-linux-gnu/bitcoin-93a70a42d30f-aarch64-linux-gnu.tar.gz
9855573d
...
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3358249438)
My Guix build:
```
aarch64
ff3ab842fd36d8c51e76fa87d80e460e3a26fca8641543f01be1b7b62eea99d8 guix-build-93a70a42d30f/output/aarch64-linux-gnu/SHA256SUMS.part
d8e022a8f8794df818cf682e75da7ca2f633e5954cd306cf14cd06f62bc3405d guix-build-93a70a42d30f/output/aarch64-linux-gnu/bitcoin-93a70a42d30f-aarch64-linux-gnu-debug.tar.gz
c7ade0382153220a35b2d59a04f4c6578e40957898c468e0040555c49508039d guix-build-93a70a42d30f/output/aarch64-linux-gnu/bitcoin-93a70a42d30f-aarch64-linux-gnu.tar.gz
9855573d
...
💬 VegArchie commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3358306446)
**Do to the overwhelming opposition to the OPRETURN changes** in v30, and the unwarranted bending of the knee toward side chains, **please remove any OPRETURN changes in v30** until a better, more acceptable solution for mempool accuracy or side chains can be developed.
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3358306446)
**Do to the overwhelming opposition to the OPRETURN changes** in v30, and the unwarranted bending of the knee toward side chains, **please remove any OPRETURN changes in v30** until a better, more acceptable solution for mempool accuracy or side chains can be developed.
💬 Dorex45 commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3358336901)
@VegArchie
I disagree. The OP_RETURN changes in v30 are not about weakening Bitcoin but about strengthening its utility for scaling. By refining how OP_RETURN is handled, we improve data efficiency and open up better pathways for sidechains and layer-2 solutions to anchor securely on Bitcoin. Removing these changes would only slow down progress for scaling and innovation, which are essential if Bitcoin is to serve more users globally.
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3358336901)
@VegArchie
I disagree. The OP_RETURN changes in v30 are not about weakening Bitcoin but about strengthening its utility for scaling. By refining how OP_RETURN is handled, we improve data efficiency and open up better pathways for sidechains and layer-2 solutions to anchor securely on Bitcoin. Removing these changes would only slow down progress for scaling and innovation, which are essential if Bitcoin is to serve more users globally.
🤔 fjahr reviewed a pull request: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3284917169)
Looks very good to me, I am just curious about the two questions I have from my last pass, but I am happy to ACK once these are addressed (with code or comment).
FWIW, I also didn't like that the functional test only checked the happy path, so I drafted some tests for failure scenarios here: https://github.com/fjahr/bitcoin/commit/889af13325a0f5fa16c2da6d71808f5fa6f15a1d I will open this as a follow-up after merge.
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3284917169)
Looks very good to me, I am just curious about the two questions I have from my last pass, but I am happy to ACK once these are addressed (with code or comment).
FWIW, I also didn't like that the functional test only checked the happy path, so I drafted some tests for failure scenarios here: https://github.com/fjahr/bitcoin/commit/889af13325a0f5fa16c2da6d71808f5fa6f15a1d I will open this as a follow-up after merge.
💬 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_r2395059707)
Doesn't this mean that we can never recover from a situation where we have some partial sig but not all pubnonces? I guess this situation is prevented by the calling code but still, I would have expected here to rather check if any new partial sigs were added in the code above just now because that would imply the necessary pubnonce for that particular partial sig was available.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2395059707)
Doesn't this mean that we can never recover from a situation where we have some partial sig but not all pubnonces? I guess this situation is prevented by the calling code but still, I would have expected here to rather check if any new partial sigs were added in the code above just now because that would imply the necessary pubnonce for that particular partial sig was available.
💬 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