💬 glozow commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519792868)
I think these functions should actually require the lock to already be held. They are only called from `MemPoolAccept` so this is always true.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519792868)
I think these functions should actually require the lock to already be held. They are only called from `MemPoolAccept` so this is always true.
💬 glozow commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519723491)
cec26203732ce3883e7e014f954ccd73c3c0099d: The function signature in chain.h still calls this `descendants`
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519723491)
cec26203732ce3883e7e014f954ccd73c3c0099d: The function signature in chain.h still calls this `descendants`
💬 glozow commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519646193)
Now that the feerate diagram outputs weight, the sanity check can be stronger:
```diff
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 472ed581145..0be261ad964 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -418,6 +418,8 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
uint64_t checkTotal = 0;
CAmount check_total_fee{0};
+ CAmount check_total_modified_fee{0};
+ int32_t check_total_adjusted_weight{0};
uint
...
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519646193)
Now that the feerate diagram outputs weight, the sanity check can be stronger:
```diff
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 472ed581145..0be261ad964 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -418,6 +418,8 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
uint64_t checkTotal = 0;
CAmount check_total_fee{0};
+ CAmount check_total_modified_fee{0};
+ int32_t check_total_adjusted_weight{0};
uint
...
💬 glozow commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519846231)
Is this supposed to be the name of the param?
```suggestion
uint256 hash = ParseHashV(request.params[0], "txid");
```
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519846231)
Is this supposed to be the name of the param?
```suggestion
uint256 hash = ParseHashV(request.params[0], "txid");
```
💬 glozow commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519835223)
nit: missing parens
Also, I wonder if it would be confusing that an RPC result named "weight" is adjusted in some RPCs but not in others... should we be calling this "adjusted_weight" ? Or perhaps "clusterweight" and "chunkweight" are enough to indicate that this is in the mempool context and thus not just BIP 141 weight.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519835223)
nit: missing parens
Also, I wonder if it would be confusing that an RPC result named "weight" is adjusted in some RPCs but not in others... should we be calling this "adjusted_weight" ? Or perhaps "clusterweight" and "chunkweight" are enough to indicate that this is in the mempool context and thus not just BIP 141 weight.
💬 glozow commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519515632)
```suggestion
// Use ChangeSet interface to check whether the cluster count
// limits would be violated. Note that the changeset will be destroyed
// when it goes out of scope.
```
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2519515632)
```suggestion
// Use ChangeSet interface to check whether the cluster count
// limits would be violated. Note that the changeset will be destroyed
// when it goes out of scope.
```
💬 glozow commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2535383593)
57cb99c82504693601709eeb47eba9cd7ab7e387 requires that `GetCluster` output txns in a deterministic order, which could be worth documenting.
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2535383593)
57cb99c82504693601709eeb47eba9cd7ab7e387 requires that `GetCluster` output txns in a deterministic order, which could be worth documenting.
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3560813571)
@glozow I actually have one concern around the value of POST_CHANGE_WORK; I think I'm going to suggest we drop it to `5 * ACCEPTABLE_ITERS` to avoid using too much CPU when encountering a cluster that our current linearization code cannot quickly find an optimal ordering.
(Likely this concern will go away completely with SFL (#32545), so I'm doing some benchmarks with and without that code to make sure things look good.)
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3560813571)
@glozow I actually have one concern around the value of POST_CHANGE_WORK; I think I'm going to suggest we drop it to `5 * ACCEPTABLE_ITERS` to avoid using too much CPU when encountering a cluster that our current linearization code cannot quickly find an optimal ordering.
(Likely this concern will go away completely with SFL (#32545), so I'm doing some benchmarks with and without that code to make sure things look good.)
💬 glozow commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2548163119)
Some documentation that could be worth having in doc/policy/ ?
### Fee and Size Terminology in Mempool Policy
- Each transaction has a **weight** and virtual size as defined in BIP 141 (different from serialized size for witness
transactions as witness data is discounted and the value is rounded up to the nearest integer).
- In the RPCs, "weight" refers to the weight as defined in BIP 141.
- A transaction has a **sigops size**, defined as its sigop cost multiplied by the node's
...
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2548163119)
Some documentation that could be worth having in doc/policy/ ?
### Fee and Size Terminology in Mempool Policy
- Each transaction has a **weight** and virtual size as defined in BIP 141 (different from serialized size for witness
transactions as witness data is discounted and the value is rounded up to the nearest integer).
- In the RPCs, "weight" refers to the weight as defined in BIP 141.
- A transaction has a **sigops size**, defined as its sigop cost multiplied by the node's
...
⚠️ byLAEV opened an issue: "Maximum Bitcoin price based on LAEV "
(https://github.com/bitcoin-core/gui/issues/912)
@gregwebs @ldenman @casey @eklitzke @rion @diegoviola @diegoviola @richardkiss @Empact @lian @petertodd @Sjors @witten @nothingmuch @vegard @takinbo
It's very simple, the electronic banking money system has a functional basis and worldwide adoption.Bitcoin was created to be electronic money exactly like traditional bank money with the only difference bein
...
(https://github.com/bitcoin-core/gui/issues/912)
@gregwebs @ldenman @casey @eklitzke @rion @diegoviola @diegoviola @richardkiss @Empact @lian @petertodd @Sjors @witten @nothingmuch @vegard @takinbo
It's very simple, the electronic banking money system has a functional basis and worldwide adoption.Bitcoin was created to be electronic money exactly like traditional bank money with the only difference bein
...
⚠️ byLAEV opened an issue: "100k Bitcoin price: compelling fundamentals "
(https://github.com/bitcoin-core/gui/issues/913)
@gregwebs @ldenman @casey @eklitzke @rion @diegoviola @diegoviola @richardkiss @Empact @lian @petertodd @Sjors @witten @nothingmuch @vegard @takinbo
)
It's very simple, the electronic banking money system has a functional basis and worldwide adoption.Bitcoin was created to be electronic money exactly like traditional bank money with the only difference being that it prevents double spending and is decentralized.Therefore, the value of the satoshi should not be linked to values like 0.001 or 0.
...
(https://github.com/bitcoin-core/gui/issues/913)
@gregwebs @ldenman @casey @eklitzke @rion @diegoviola @diegoviola @richardkiss @Empact @lian @petertodd @Sjors @witten @nothingmuch @vegard @takinbo
)
It's very simple, the electronic banking money system has a functional basis and worldwide adoption.Bitcoin was created to be electronic money exactly like traditional bank money with the only difference being that it prevents double spending and is decentralized.Therefore, the value of the satoshi should not be linked to values like 0.001 or 0.
...
💬 Sammie05 commented on pull request "ci: Run GUI unit tests in cross-Windows task":
(https://github.com/bitcoin/bitcoin/pull/33919#issuecomment-3560855219)
The workflow correctly attempts to execute the GUI test binary, ./bin/test_bitcoin-qt.exe. but the CI logs do not show any Qt test output and there are no lines such as ********* Start testing of <TestSuite> *********
PASS / FAIL entries
********* Finished testing of <TestSuite> *********
It would be helpful to confirm that test_bitcoin-qt.exe is executing as intended in the cross-Windows environment.
(https://github.com/bitcoin/bitcoin/pull/33919#issuecomment-3560855219)
The workflow correctly attempts to execute the GUI test binary, ./bin/test_bitcoin-qt.exe. but the CI logs do not show any Qt test output and there are no lines such as ********* Start testing of <TestSuite> *********
PASS / FAIL entries
********* Finished testing of <TestSuite> *********
It would be helpful to confirm that test_bitcoin-qt.exe is executing as intended in the cross-Windows environment.
💬 Sammie05 commented on pull request "test: Retry download in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/33915#issuecomment-3561716043)
I verified the retry mechanism by testing both successful downloads (v25.0) and failure scenarios (invalid_version_123), the retry logic correctly activates on HTTP 404 errors and provides clear error message
Fetching: https://bitcoincore.org/bin/bitcoin-core-nvalid_version_123/bitcoin-nvalid_version_123-osx64.tar.gz
Download failed: HTTP Error 404: Not Found
Retrying download after failure ...
Download failed a second time: HTTP Error 404: Not Found
(https://github.com/bitcoin/bitcoin/pull/33915#issuecomment-3561716043)
I verified the retry mechanism by testing both successful downloads (v25.0) and failure scenarios (invalid_version_123), the retry logic correctly activates on HTTP 404 errors and provides clear error message
Fetching: https://bitcoincore.org/bin/bitcoin-core-nvalid_version_123/bitcoin-nvalid_version_123-osx64.tar.gz
Download failed: HTTP Error 404: Not Found
Retrying download after failure ...
Download failed a second time: HTTP Error 404: Not Found
💬 ajtowns commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#discussion_r2548796423)
I'm working on making it be:
* have node/miner generate two <4MW `CBlock` results, rather than a single <8MW one.
* send a `TEMPLATE` message with both blocks encoded as compact blocks
* request missing txns from the first block; request missing txns from the second block
* receive missing txns from the first block
* receive missing txns from the second block; add the txs from both blocks as that peer's template
Little bit fiddly to make it all work, but I'm not seeing any showst
...
(https://github.com/bitcoin/bitcoin/pull/33191#discussion_r2548796423)
I'm working on making it be:
* have node/miner generate two <4MW `CBlock` results, rather than a single <8MW one.
* send a `TEMPLATE` message with both blocks encoded as compact blocks
* request missing txns from the first block; request missing txns from the second block
* receive missing txns from the first block
* receive missing txns from the second block; add the txs from both blocks as that peer's template
Little bit fiddly to make it all work, but I'm not seeing any showst
...
💬 maflcko commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#discussion_r2548811262)
It could make sense to ask Ubuntu to import g++-mingw-w64-ucrt64 from Debian, at least for the 26.04 release.
My hope was that one toolchain could be dropped while adding the new one relatively quickly, or even at the same time.
Unrelated, I wonder what all the major benefits of ucrt are. Is it just the x-modifier in fopen calls?
(https://github.com/bitcoin/bitcoin/pull/33857#discussion_r2548811262)
It could make sense to ask Ubuntu to import g++-mingw-w64-ucrt64 from Debian, at least for the 26.04 release.
My hope was that one toolchain could be dropped while adding the new one relatively quickly, or even at the same time.
Unrelated, I wonder what all the major benefits of ucrt are. Is it just the x-modifier in fopen calls?
💬 janb84 commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2548815278)
Weird, dev-mode should have those flags on.
Your build is reporting this [config](https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19550555322/job/55988016371#step:6:1874).
```sh
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
```
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2548815278)
Weird, dev-mode should have those flags on.
Your build is reporting this [config](https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19550555322/job/55988016371#step:6:1874).
```sh
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
```
💬 hebasto commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#discussion_r2549088989)
> It could make sense to ask Ubuntu to import g++-mingw-w64-ucrt64 from Debian, at least for the 26.04 release.
It appears to be a WIP: https://packages.ubuntu.com/search?keywords=ucrt
> Unrelated, I wonder what all the major benefits of ucrt are. Is it just the x-modifier in fopen calls?
From https://learn.microsoft.com/en-us/cpp/porting/upgrade-your-code-to-the-universal-crt:
> Many functions were added or updated in the UCRT to improve ISO C99 conformance, and to address code qualit
...
(https://github.com/bitcoin/bitcoin/pull/33857#discussion_r2549088989)
> It could make sense to ask Ubuntu to import g++-mingw-w64-ucrt64 from Debian, at least for the 26.04 release.
It appears to be a WIP: https://packages.ubuntu.com/search?keywords=ucrt
> Unrelated, I wonder what all the major benefits of ucrt are. Is it just the x-modifier in fopen calls?
From https://learn.microsoft.com/en-us/cpp/porting/upgrade-your-code-to-the-universal-crt:
> Many functions were added or updated in the UCRT to improve ISO C99 conformance, and to address code qualit
...
🤔 janb84 reviewed a pull request: "test: Retry download in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/33915#pullrequestreview-3491910763)
ACK fad06f3bb436a97683e8248bfda1bd0d9998c899
PR adds one retry to download a prev. release.
The single retry is somewhat crude solution but a good first step to solve the intermitted download issues, good KISS sollution. If this isn't enough we can always fallback on more elegant solutions, but this also increases the complexity.
(https://github.com/bitcoin/bitcoin/pull/33915#pullrequestreview-3491910763)
ACK fad06f3bb436a97683e8248bfda1bd0d9998c899
PR adds one retry to download a prev. release.
The single retry is somewhat crude solution but a good first step to solve the intermitted download issues, good KISS sollution. If this isn't enough we can always fallback on more elegant solutions, but this also increases the complexity.
💬 maflcko commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3562209741)
review ACK 55c6a69f777ac08a14e61b98efea00e5c8f98a5f 🚏
<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: review ACK 55c6a69f777a
...
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3562209741)
review ACK 55c6a69f777ac08a14e61b98efea00e5c8f98a5f 🚏
<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: review ACK 55c6a69f777a
...
💬 hebasto commented on pull request "Add console commands for clearing output and history":
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3562241063)
https://github.com/bitcoin-core/gui/pull/882#issue-3276693433:
> Provides explicit, privacy-orientd ways to clear console history and output without altering default behavior.
> Useful in shared or long-running sessions.
I’m not convinced that building a privacy-focused feature around the idea of a “shared session” is sound. I’m not even sure what exactly qualifies as a "shared session" in this context, and if privacy is the aim, preventing such scenarios altogether seems the more appropr
...
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3562241063)
https://github.com/bitcoin-core/gui/pull/882#issue-3276693433:
> Provides explicit, privacy-orientd ways to clear console history and output without altering default behavior.
> Useful in shared or long-running sessions.
I’m not convinced that building a privacy-focused feature around the idea of a “shared session” is sound. I’m not even sure what exactly qualifies as a "shared session" in this context, and if privacy is the aim, preventing such scenarios altogether seems the more appropr
...