💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020819698)
> Any reason not to use bash + sed here?
Mostly because `sed` is not mentioned in the image [docs](https://github.com/actions/runner-images/blob/win22/20250324.3/images/windows/Windows2022-Readme.md).
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020819698)
> Any reason not to use bash + sed here?
Mostly because `sed` is not mentioned in the image [docs](https://github.com/actions/runner-images/blob/win22/20250324.3/images/windows/Windows2022-Readme.md).
💬 maflcko commented on issue "Segfault during shutdown in SendCoinsDialog::updateCoinControlState":
(https://github.com/bitcoin-core/gui/issues/862#issuecomment-2765876870)
Steps to reproduce:
* Apply diff on current master to create more (fake) block tip events for the gui
```diff
diff --git a/src/init.cpp b/src/init.cpp
index f35a547c92..7c5fc7f65f 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1885,7 +1885,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
int64_t best_block_time{};
{
LOCK(chainman.GetMutex());
- const auto& tip{*Assert(chainman.ActiveTip())};
+ auto& tip{*Assert(chainman.Ac
...
(https://github.com/bitcoin-core/gui/issues/862#issuecomment-2765876870)
Steps to reproduce:
* Apply diff on current master to create more (fake) block tip events for the gui
```diff
diff --git a/src/init.cpp b/src/init.cpp
index f35a547c92..7c5fc7f65f 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1885,7 +1885,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
int64_t best_block_time{};
{
LOCK(chainman.GetMutex());
- const auto& tip{*Assert(chainman.ActiveTip())};
+ auto& tip{*Assert(chainman.Ac
...
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020837144)
Wouldn't it restore an invalidated cache when the affected files have been modified in a meaningful way?
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020837144)
Wouldn't it restore an invalidated cache when the affected files have been modified in a meaningful way?
💬 ajtowns commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2765898369)
> I suggest we reconsider this. To add on to the earlier points in favor of this approach, I ran into "tx-size-small" when testing new policies with packages: a 1-in-1-out child spending the p2a of a parent can be less than 64 bytes.
That's easily fixed by having the 1-out be a 0sat OP_RETURN pushing three bytes of data... I remain Concept and Approach NACK on this, supporting 60-63 byte txs but not 64 byte txs just seems like it's asking for trouble. ie, just add the extra bytes to your outp
...
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2765898369)
> I suggest we reconsider this. To add on to the earlier points in favor of this approach, I ran into "tx-size-small" when testing new policies with packages: a 1-in-1-out child spending the p2a of a parent can be less than 64 bytes.
That's easily fixed by having the 1-out be a 0sat OP_RETURN pushing three bytes of data... I remain Concept and Approach NACK on this, supporting 60-63 byte txs but not 64 byte txs just seems like it's asking for trouble. ie, just add the extra bytes to your outp
...
💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2765906183)
> Would be nice to fix locale warnings (https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2764240416), either here or in follow-up.
To be clear, i don't think anything should be done in this PR. Adding `glibc-locales`, a large package with the locales for all supported languages to the closure, just to shut up a warning is not worth it.
As mentioned [here](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2763240357), default `C.UTF-8` locale support will be (or already i
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2765906183)
> Would be nice to fix locale warnings (https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2764240416), either here or in follow-up.
To be clear, i don't think anything should be done in this PR. Adding `glibc-locales`, a large package with the locales for all supported languages to the closure, just to shut up a warning is not worth it.
As mentioned [here](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2763240357), default `C.UTF-8` locale support will be (or already i
...
💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020890145)
In this case, I'd say the depends system should detect it and discard the cache. Otherwise, developers or users may run into the same issue.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020890145)
In this case, I'd say the depends system should detect it and discard the cache. Otherwise, developers or users may run into the same issue.
🤔 janb84 reviewed a pull request: "tests: improves tapscript unit tests"
(https://github.com/bitcoin/bitcoin/pull/31640#pullrequestreview-2729031093)
Re ACK [62b6071](https://github.com/bitcoin/bitcoin/commit/62b6071fd6ac2aad2fc42f135b3df662ab0eee7a)
(https://github.com/bitcoin/bitcoin/pull/31640#pullrequestreview-2729031093)
Re ACK [62b6071](https://github.com/bitcoin/bitcoin/commit/62b6071fd6ac2aad2fc42f135b3df662ab0eee7a)
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020896290)
Could you elaborate on your concern?
A cache key includes the hash of all files in `depends`. Any change will invalidate the cache. Right?
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020896290)
Could you elaborate on your concern?
A cache key includes the hash of all files in `depends`. Any change will invalidate the cache. Right?
⚠️ dergoegge opened an issue: "validation: `CheckBlockIndex` crashes during block reconsideration"
(https://github.com/bitcoin/bitcoin/issues/32173)
Functional test to reproduce:
```python
from test_framework.test_framework import BitcoinTestFramework
class CheckBlockIndexBug(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
self.num_nodes = 1
def run_test(self):
self.generatetoaddress(self.nodes[0], 1, "2N9hLwkSqr1cPQAPxbrGVUjxyjD11G2e1he");
hashes = self.generatetoaddress(self.nodes[0], 1, "2N9hLwkSqr1cPQAPxbrGVUjxyjD11G2e1he");
self.generatetoaddress(self.nod
...
(https://github.com/bitcoin/bitcoin/issues/32173)
Functional test to reproduce:
```python
from test_framework.test_framework import BitcoinTestFramework
class CheckBlockIndexBug(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
self.num_nodes = 1
def run_test(self):
self.generatetoaddress(self.nodes[0], 1, "2N9hLwkSqr1cPQAPxbrGVUjxyjD11G2e1he");
hashes = self.generatetoaddress(self.nodes[0], 1, "2N9hLwkSqr1cPQAPxbrGVUjxyjD11G2e1he");
self.generatetoaddress(self.nod
...
💬 ismaelsadeeq commented on issue "RPC: `getblockstats` might not return the *effective* percentile fee rate":
(https://github.com/bitcoin/bitcoin/issues/31140#issuecomment-2766001556)
> A more efficient approach would be to persist previous block package data. With this approach, I don't see any reason to support linearizing non-mempool transactions in MiniMiner for now. While this would increase resource usage, I believe the trade-off is justified when compared to the complexity of linearizing clusters on the fly.
> Having this package data available from disk would be very helpful for both this issue and https://github.com/bitcoin/bitcoin/pull/30079.
For #30079, I don't t
...
(https://github.com/bitcoin/bitcoin/issues/31140#issuecomment-2766001556)
> A more efficient approach would be to persist previous block package data. With this approach, I don't see any reason to support linearizing non-mempool transactions in MiniMiner for now. While this would increase resource usage, I believe the trade-off is justified when compared to the complexity of linearizing clusters on the fly.
> Having this package data available from disk would be very helpful for both this issue and https://github.com/bitcoin/bitcoin/pull/30079.
For #30079, I don't t
...
💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020907384)
The current CI persistent workers, or a local CI run, or a local depends build, doesn't invalidate the depends cache on any change in any file. If this could lead to problems, I'd say it should be fixed in depends itself. Otherwise, almost every place where depends is called, will have to apply the workaround here.
But this is just a nit here.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020907384)
The current CI persistent workers, or a local CI run, or a local depends build, doesn't invalidate the depends cache on any change in any file. If this could lead to problems, I'd say it should be fixed in depends itself. Otherwise, almost every place where depends is called, will have to apply the workaround here.
But this is just a nit here.
🤔 hodlinator reviewed a pull request: "fuzz: Make partially_downloaded_block more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32158#pullrequestreview-2728650579)
Concept ACK fa1e2995d9996641e79f92549e99a6b37e36d140
(https://github.com/bitcoin/bitcoin/pull/32158#pullrequestreview-2728650579)
Concept ACK fa1e2995d9996641e79f92549e99a6b37e36d140
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020666474)
nit: `[]`-brackets are commonly used for optional arguments.
```suggestion
Usage: program ./build_dir ./qa-assets/fuzz_corpora fuzz_target_name [parallelism={DEFAULT_PAR}]
```
Might be nice to standardize on `-jN`, but keeping the logic simple is also good.
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020666474)
nit: `[]`-brackets are commonly used for optional arguments.
```suggestion
Usage: program ./build_dir ./qa-assets/fuzz_corpora fuzz_target_name [parallelism={DEFAULT_PAR}]
```
Might be nice to standardize on `-jN`, but keeping the logic simple is also good.
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020707730)
I'm worried this means we get decreased fuzz-coverage. Maybe could introduce another var?
```suggestion
.worker_threads_num = G_FUZZING && G_DETERMINISTIC? 0 : 2,
```
Although being able to reproduce fuzz failures is also nice. :\
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020707730)
I'm worried this means we get decreased fuzz-coverage. Maybe could introduce another var?
```suggestion
.worker_threads_num = G_FUZZING && G_DETERMINISTIC? 0 : 2,
```
Although being able to reproduce fuzz failures is also nice. :\
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020914099)
Appreciate you adding the "[N/M]" output, but it's spammed away by fuzz-output even on success. Suggest capturing stdio/stderr and only output them on failure: ea3e89e48250013ea818abcefd7be8e72d31f23d
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020914099)
Appreciate you adding the "[N/M]" output, but it's spammed away by fuzz-output even on success. Suggest capturing stdio/stderr and only output them on failure: ea3e89e48250013ea818abcefd7be8e72d31f23d
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020911351)
Found a way to reduce this code by 14 lines. What do you think about f8a0a32280d7636135f6821401d7f3b18d10476b?
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020911351)
Found a way to reduce this code by 14 lines. What do you think about f8a0a32280d7636135f6821401d7f3b18d10476b?
💬 maflcko commented on pull request "doc/descriptors.md: Update next halvening heights":
(https://github.com/bitcoin/bitcoin/pull/32168#discussion_r2020919177)
Not sure it is appropriate to have to update this doc for every halving, also the descriptor uses tpub, which is for test networks? So the halving heights are unclear anyway. Finally, the checksum doesn't match, so this will fail regardless.
(https://github.com/bitcoin/bitcoin/pull/32168#discussion_r2020919177)
Not sure it is appropriate to have to update this doc for every halving, also the descriptor uses tpub, which is for test networks? So the halving heights are unclear anyway. Finally, the checksum doesn't match, so this will fail regardless.
💬 hodlinator commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020926999)
Would be happy to be listed as commit co-author if you take this or the output spam reduction (https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020914099) as this is partially inspired by https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2001916136.
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020926999)
Would be happy to be listed as commit co-author if you take this or the output spam reduction (https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2020914099) as this is partially inspired by https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2001916136.
💬 instagibbs commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020930001)
can you give a concrete suggestion with a diff?
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020930001)
can you give a concrete suggestion with a diff?
💬 instagibbs commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020933005)
tried being a little more explicit, but I'm punting larger refactors for Future Work
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020933005)
tried being a little more explicit, but I'm punting larger refactors for Future Work
💬 ajtowns commented on issue "RPC: `getblockstats` might not return the *effective* percentile fee rate":
(https://github.com/bitcoin/bitcoin/issues/31140#issuecomment-2766061375)
> However, the fee rate of a transaction at a given percentile may not reflect its _effective_ fee rate, as it could be fee-bumped by some descendant transactions.
The block's txs are put into `feerate_array` in order, but `CalculatePercentilesByWeight` sorts them by feerate, so if you have a block with txs at fee rates `[5, 4, 3, 100, 2, 2, 1, 1, 1]` it will be treated as `[100, 5, 4, 3, 2, 2, 1, 1, 1]`, so even if the "100" is a child paying for the "5" and "4" txs, it will come first when ca
...
(https://github.com/bitcoin/bitcoin/issues/31140#issuecomment-2766061375)
> However, the fee rate of a transaction at a given percentile may not reflect its _effective_ fee rate, as it could be fee-bumped by some descendant transactions.
The block's txs are put into `feerate_array` in order, but `CalculatePercentilesByWeight` sorts them by feerate, so if you have a block with txs at fee rates `[5, 4, 3, 100, 2, 2, 1, 1, 1]` it will be treated as `[100, 5, 4, 3, 2, 2, 1, 1, 1]`, so even if the "100" is a child paying for the "5" and "4" txs, it will come first when ca
...