π¬ marcofleon commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3329445838)
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
I think this warning makes sense to remove for v30 if thereβs not yet a clear timeline for deprecation. I donβt think the wording matters too much for now, and can be decided on later along with a deprecation plan and better documentation wrt this setting.
Could be good to set various arbitrary values on some monitoring nodes and see what the impacts are (potentially more stale blocks?). Ultimately, if users, miners included, want to set this to
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3329445838)
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
I think this warning makes sense to remove for v30 if thereβs not yet a clear timeline for deprecation. I donβt think the wording matters too much for now, and can be decided on later along with a deprecation plan and better documentation wrt this setting.
Could be good to set various arbitrary values on some monitoring nodes and see what the impacts are (potentially more stale blocks?). Ultimately, if users, miners included, want to set this to
...
π ismaelsadeeq opened a pull request: "bugfix: miner: fix `addPackageTxs` unsigned integer overflow"
(https://github.com/bitcoin/bitcoin/pull/33475)
This PR fixes an unsigned integer overflow in the `addPackageTxs` method of the `BlockAssembler`.
The overflow is a rare edge case that might occur on master when a miner reserves 2000 WU and wants to create an block to be empty.
i.e, by starting with `-blockmaxweight=2000`, `-blockreservedweight=2000`, or just `blockmaxweight=2000`, and then calling the mining interface `createNewBlock` with `blockReservedWeight` set to `2000`.
Instead of bailing out after going through transactions eq
...
(https://github.com/bitcoin/bitcoin/pull/33475)
This PR fixes an unsigned integer overflow in the `addPackageTxs` method of the `BlockAssembler`.
The overflow is a rare edge case that might occur on master when a miner reserves 2000 WU and wants to create an block to be empty.
i.e, by starting with `-blockmaxweight=2000`, `-blockreservedweight=2000`, or just `blockmaxweight=2000`, and then calling the mining interface `createNewBlock` with `blockReservedWeight` set to `2000`.
Instead of bailing out after going through transactions eq
...
π¬ jesterhodl commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3329617679)
> > > Sure, see https://github.com/maflcko/b-c-gui-translations-review/tree/99bf41c4e5ab74eee8c248b240e8e940e47e09ec/reviews
> >
> >
> > Friendly ping to coordinators for addressing issues:
> >
> > * @sr-gi -- Catalan (ca)
> > * @ostruvek -- Czech (cs)
> > * @pryds -- Danish (da)
> > * @laanwj @sipa -- Dutch (nl)
> > * @Emzy -- German (de)
> > * @cryptomeow -- Greek (el)
> > * @jesterhodl -- Polish (pl)
> >
> > UPD. French (fr) and Spanish (es) coordinators have been notified vi
...
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3329617679)
> > > Sure, see https://github.com/maflcko/b-c-gui-translations-review/tree/99bf41c4e5ab74eee8c248b240e8e940e47e09ec/reviews
> >
> >
> > Friendly ping to coordinators for addressing issues:
> >
> > * @sr-gi -- Catalan (ca)
> > * @ostruvek -- Czech (cs)
> > * @pryds -- Danish (da)
> > * @laanwj @sipa -- Dutch (nl)
> > * @Emzy -- German (de)
> > * @cryptomeow -- Greek (el)
> > * @jesterhodl -- Polish (pl)
> >
> > UPD. French (fr) and Spanish (es) coordinators have been notified vi
...
π¬ maflcko commented on pull request "Backport Cirrus runners to 28.x":
(https://github.com/bitcoin/bitcoin/pull/33406#issuecomment-3329647242)
lgtm ACK ea4e0aa8c4 π₯
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK ea4e0aa8c4 π₯
gzSENNVTLprDXOwKFeI2v4to/hZq+ZG
...
(https://github.com/bitcoin/bitcoin/pull/33406#issuecomment-3329647242)
lgtm ACK ea4e0aa8c4 π₯
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK ea4e0aa8c4 π₯
gzSENNVTLprDXOwKFeI2v4to/hZq+ZG
...
π¬ john-moffett commented on pull request "rpc: addpeeraddress: throw on invalid IP":
(https://github.com/bitcoin/bitcoin/pull/33430#issuecomment-3329686831)
Thanks, all!
@vasild
> As an alternative, we could use the `error` field in the response instead of throwing an exception.
I tried to use the same pattern as `setban` does, which I think has a clean boundary between an invalid input and a problem that occurred despite valid input. To me, that category separation makes sense.
@pablomartin4btc
> Would it be useful for the user to differentiate from an empty address vs a provided invalid IP (which obviously includes an empty value)
...
(https://github.com/bitcoin/bitcoin/pull/33430#issuecomment-3329686831)
Thanks, all!
@vasild
> As an alternative, we could use the `error` field in the response instead of throwing an exception.
I tried to use the same pattern as `setban` does, which I think has a clean boundary between an invalid input and a problem that occurred despite valid input. To me, that category separation makes sense.
@pablomartin4btc
> Would it be useful for the user to differentiate from an empty address vs a provided invalid IP (which obviously includes an empty value)
...
π¬ john-moffett commented on pull request "rpc: Always return per-wtxid entries in submitpackage tx-results":
(https://github.com/bitcoin/bitcoin/pull/33427#discussion_r2376310092)
Apologies if it was too minor to refactor! Will try to keep it more surgical in the future for these types of touch-up PRs.
(https://github.com/bitcoin/bitcoin/pull/33427#discussion_r2376310092)
Apologies if it was too minor to refactor! Will try to keep it more surgical in the future for these types of touch-up PRs.
π¬ katesalazar commented on pull request "Fix dark mode detection on Linux":
(https://github.com/bitcoin-core/gui/pull/895#issuecomment-3329734282)
Ah, nice and small, thanks. I have a couple of questions.
This is in your code:
> This must be done before creating the QApplication
Does this mean window won't change live when theme changes, on Linux?
This is in your post:
> unlike macOS where it works out of the box.
Does this mean window will change live when theme changes, on macOS?
(https://github.com/bitcoin-core/gui/pull/895#issuecomment-3329734282)
Ah, nice and small, thanks. I have a couple of questions.
This is in your code:
> This must be done before creating the QApplication
Does this mean window won't change live when theme changes, on Linux?
This is in your post:
> unlike macOS where it works out of the box.
Does this mean window will change live when theme changes, on macOS?
π€ hebasto reviewed a pull request: "depends: static libxcb-cursor"
(https://github.com/bitcoin/bitcoin/pull/33434#pullrequestreview-3263686528)
My Guix builds for Linux hosts on both `aarch64` and `x86_64` platforms:
```
cf7f86d91288477c11afdc767969d6b19b0ba1e62c73924e72b22a2527374653 guix-build-eca50854e1cb/output/aarch64-linux-gnu/SHA256SUMS.part
5afe50eb76079f96a5129b5e51e2ede657f10c5031abe24a875410f006c55511 guix-build-eca50854e1cb/output/aarch64-linux-gnu/bitcoin-eca50854e1cb-aarch64-linux-gnu-debug.tar.gz
ba6f7937c5d333e64560abcd95dbb3856ceaa43fbcde766ebcaed1f8e23f2a7e guix-build-eca50854e1cb/output/aarch64-linux-gnu/bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/33434#pullrequestreview-3263686528)
My Guix builds for Linux hosts on both `aarch64` and `x86_64` platforms:
```
cf7f86d91288477c11afdc767969d6b19b0ba1e62c73924e72b22a2527374653 guix-build-eca50854e1cb/output/aarch64-linux-gnu/SHA256SUMS.part
5afe50eb76079f96a5129b5e51e2ede657f10c5031abe24a875410f006c55511 guix-build-eca50854e1cb/output/aarch64-linux-gnu/bitcoin-eca50854e1cb-aarch64-linux-gnu-debug.tar.gz
ba6f7937c5d333e64560abcd95dbb3856ceaa43fbcde766ebcaed1f8e23f2a7e guix-build-eca50854e1cb/output/aarch64-linux-gnu/bitcoi
...
π€ glozow reviewed a pull request: "bugfix: miner: fix `addPackageTxs` unsigned integer overflow"
(https://github.com/bitcoin/bitcoin/pull/33475#pullrequestreview-3263812697)
nice, utACK b807dfcdc5929c314d43b790c9e705d5bf0a86e8
(https://github.com/bitcoin/bitcoin/pull/33475#pullrequestreview-3263812697)
nice, utACK b807dfcdc5929c314d43b790c9e705d5bf0a86e8
π€ furszy reviewed a pull request: "bugfix: miner: fix `addPackageTxs` unsigned integer overflow"
(https://github.com/bitcoin/bitcoin/pull/33475#pullrequestreview-3263830391)
Code ACK b807dfcdc5929c314d43b790c9e705d5bf0a86e8
(https://github.com/bitcoin/bitcoin/pull/33475#pullrequestreview-3263830391)
Code ACK b807dfcdc5929c314d43b790c9e705d5bf0a86e8
π¬ fjahr commented on pull request "rpc: Fix dumptxoutset rollback with competing forks":
(https://github.com/bitcoin/bitcoin/pull/33444#issuecomment-3329887782)
This seems to implement the same logic that I used in #31117 but this turned out to be too fragile to be seriously considered for merging there. In this context it may work well enough for mainnet because there are not a lot of forks but we have been discussing an alternative approach for a while that does not rely on `invalidateblock`/`reconsiderblock` and this would solve the competing forks problem as well. I have worked on this idea for #31117 primarily but I will open a PR with that approac
...
(https://github.com/bitcoin/bitcoin/pull/33444#issuecomment-3329887782)
This seems to implement the same logic that I used in #31117 but this turned out to be too fragile to be seriously considered for merging there. In this context it may work well enough for mainnet because there are not a lot of forks but we have been discussing an alternative approach for a while that does not rely on `invalidateblock`/`reconsiderblock` and this would solve the competing forks problem as well. I have worked on this idea for #31117 primarily but I will open a PR with that approac
...
π¬ glozow commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2376540950)
This fails `test_unspent_ephemeral` for me when applied on 00c02b42c61
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2376540950)
This fails `test_unspent_ephemeral` for me when applied on 00c02b42c61
π¬ IdotMaster1 commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3330009658)
Concept ACK, but turn the default OP_RETURN back to normal.
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3330009658)
Concept ACK, but turn the default OP_RETURN back to normal.
π fanquake merged a pull request: "Backport Cirrus runners to 28.x"
(https://github.com/bitcoin/bitcoin/pull/33406)
(https://github.com/bitcoin/bitcoin/pull/33406)
π¬ enirox001 commented on pull request "rpc: Fix dumptxoutset rollback with competing forks":
(https://github.com/bitcoin/bitcoin/pull/33444#issuecomment-3330024209)
Thanks for the review and context @fjahr . I wasn't aware of that prior work.
You're right that this approach has some fragility concerns. I'd be very interested to see your alternative approach that avoids invalidateblock/reconsiderblock altogether, as that does sound more robust.
Regarding the test failure - let me investigate and fix that. Would you prefer I:
1. Fix the test and keep this PR open for comparison with your upcoming alternative approach, or
2. Close this PR and wait fo
...
(https://github.com/bitcoin/bitcoin/pull/33444#issuecomment-3330024209)
Thanks for the review and context @fjahr . I wasn't aware of that prior work.
You're right that this approach has some fragility concerns. I'd be very interested to see your alternative approach that avoids invalidateblock/reconsiderblock altogether, as that does sound more robust.
Regarding the test failure - let me investigate and fix that. Would you prefer I:
1. Fix the test and keep this PR open for comparison with your upcoming alternative approach, or
2. Close this PR and wait fo
...
π¬ instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2376581403)
ah right, it will fail with in-mempool sibling because package version will simply fail when it encounters a "parent already has child in mempool" condition it's not meant for RBF cases
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2376581403)
ah right, it will fail with in-mempool sibling because package version will simply fail when it encounters a "parent already has child in mempool" condition it's not meant for RBF cases
π¬ glozow commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2376602411)
Noting that this check is redundant with `ReplacementChecks`, since `CalculateChunksForRBF` also calls it before calculating the feerate diagram. I think checking twice is fine, but that one has a slightly different error message that should be unified imo. See this test:
```
diff --git a/test/functional/mempool_cluster.py b/test/functional/mempool_cluster.py
index 3da8b477a2f..8bcc9b2fe65 100755
--- a/test/functional/mempool_cluster.py
+++ b/test/functional/mempool_cluster.py
@@ -4,6 +4
...
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2376602411)
Noting that this check is redundant with `ReplacementChecks`, since `CalculateChunksForRBF` also calls it before calculating the feerate diagram. I think checking twice is fine, but that one has a slightly different error message that should be unified imo. See this test:
```
diff --git a/test/functional/mempool_cluster.py b/test/functional/mempool_cluster.py
index 3da8b477a2f..8bcc9b2fe65 100755
--- a/test/functional/mempool_cluster.py
+++ b/test/functional/mempool_cluster.py
@@ -4,6 +4
...
π¬ plebhash commented on issue "`bitcoin-node` is unkillable after mining IPC connection is established":
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3330050492)
after some more progress on my Rust code, I'm no longer waiting on `waitTipChanged`, since I can get away with only waiting on `waitNext`
after these changes, I'm no longer observing `bitcoin-node` being unkillable via ctrl+c.
so this, plus the odd-looking logs above, make it seem that this is related to how `bitcoin-node` is dealing with `waitTipChanged` internally?
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3330050492)
after some more progress on my Rust code, I'm no longer waiting on `waitTipChanged`, since I can get away with only waiting on `waitNext`
after these changes, I'm no longer observing `bitcoin-node` being unkillable via ctrl+c.
so this, plus the odd-looking logs above, make it seem that this is related to how `bitcoin-node` is dealing with `waitTipChanged` internally?
π¬ glozow commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3330050997)
> It seems the new fuzz test in [16d5558](https://github.com/bitcoin/bitcoin/pull/33421/commits/16d55585aa2d26e9177734927b8446faa72570e7)
caught a minor UndefinedBehaviorSanitizer: unsigned-integer-overflow issue on master.
Nice! Sorry for the labeling noise, I meant to label #33475
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3330050997)
> It seems the new fuzz test in [16d5558](https://github.com/bitcoin/bitcoin/pull/33421/commits/16d55585aa2d26e9177734927b8446faa72570e7)
caught a minor UndefinedBehaviorSanitizer: unsigned-integer-overflow issue on master.
Nice! Sorry for the labeling noise, I meant to label #33475
π¬ maflcko commented on issue "Cleanup CFeeRate constructor (sat/vB vs BTC/kvB)":
(https://github.com/bitcoin/bitcoin/issues/23129#issuecomment-3330063270)
@polespinasa I don't think this is resolved. To explain it a bit better looking at the code:
https://github.com/bitcoin/bitcoin/blob/d41b503ae128ac36ef27e652d2935c6fe7981a79/src/wallet/rpc/spend.cpp#L223-L224
`AmountFromValue` is a function that returns a `CAmount` (i64 of satoshis). However, the `CFeeRate` constructor that is called takes a feerate per kvB:
https://github.com/bitcoin/bitcoin/blob/d41b503ae128ac36ef27e652d2935c6fe7981a79/src/policy/feerate.h#L44
So calling `CFeeRate{AmountF
...
(https://github.com/bitcoin/bitcoin/issues/23129#issuecomment-3330063270)
@polespinasa I don't think this is resolved. To explain it a bit better looking at the code:
https://github.com/bitcoin/bitcoin/blob/d41b503ae128ac36ef27e652d2935c6fe7981a79/src/wallet/rpc/spend.cpp#L223-L224
`AmountFromValue` is a function that returns a `CAmount` (i64 of satoshis). However, the `CFeeRate` constructor that is called takes a feerate per kvB:
https://github.com/bitcoin/bitcoin/blob/d41b503ae128ac36ef27e652d2935c6fe7981a79/src/policy/feerate.h#L44
So calling `CFeeRate{AmountF
...
π¬ l0rinc commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-3330072143)
I have measured the performance of V30 with previous releases (same dbcache, same assumevalid) - on an Intel i9 with an NVMe SSD and an Intel i7 with a HDD.
## Intel Core i9-9900K
Many of our optimization efforts were focusing on the underperforming, cheap hardware, so the i9 may not be representative - though running so many experiments is really slow so we often use the faster one to quickly check if an idea makes any difference.
<details>
<summary>Individual measurements for i9</sum
...
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-3330072143)
I have measured the performance of V30 with previous releases (same dbcache, same assumevalid) - on an Intel i9 with an NVMe SSD and an Intel i7 with a HDD.
## Intel Core i9-9900K
Many of our optimization efforts were focusing on the underperforming, cheap hardware, so the i9 may not be representative - though running so many experiments is really slow so we often use the faster one to quickly check if an idea makes any difference.
<details>
<summary>Individual measurements for i9</sum
...