💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1999460229)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1995939302
> Nit: `s/this a/this is a/`
Thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1999460229)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1995939302
> Nit: `s/this a/this is a/`
Thanks, fixed
💬 darosior commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r2002057033)
I figured its name already implies what it's doing and expecting (an entirely valid block, not only valid PoW), and didn't want to just add a comment like "Process a block" (akin to `PrepareBlock` right below whose docstring states "Prepare a block"). I felt only the return value was not clear from the function declaration, hence why specifying it in the docstring.
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r2002057033)
I figured its name already implies what it's doing and expecting (an entirely valid block, not only valid PoW), and didn't want to just add a comment like "Process a block" (akin to `PrepareBlock` right below whose docstring states "Prepare a block"). I felt only the return value was not clear from the function declaration, hence why specifying it in the docstring.
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2002112881)
That's a good idea! I think we can even go one step further and use `highpow_outofchain_headers` for everything, also for `cand_invalid_descendants` (at the cost of doing a few `GetAncestor()` calls, which should be cheap enough). I pushed an updated version that uses this approach.
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2002112881)
That's a good idea! I think we can even go one step further and use `highpow_outofchain_headers` for everything, also for `cand_invalid_descendants` (at the cost of doing a few `GetAncestor()` calls, which should be cheap enough). I pushed an updated version that uses this approach.
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2734888712)
[4ba2e48](https://github.com/bitcoin/bitcoin/commit/4ba2e480ffa0b77113953bee4ff5c9349e277e7e) to [fd39a64](https://github.com/bitcoin/bitcoin/commit/fd39a6420dad4cfc8bfff73f32df663da1ec00e2):
I changed the approach to `InvalidateBlock()` after the suggestion by @stickies-v: Instead of having 3 slightly different caches, now only one cache is used for everything (`highpow_outofchain_headers`). This should make the changes more simple and have a negligible overhead of a few additional `GetAncesto
...
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2734888712)
[4ba2e48](https://github.com/bitcoin/bitcoin/commit/4ba2e480ffa0b77113953bee4ff5c9349e277e7e) to [fd39a64](https://github.com/bitcoin/bitcoin/commit/fd39a6420dad4cfc8bfff73f32df663da1ec00e2):
I changed the approach to `InvalidateBlock()` after the suggestion by @stickies-v: Instead of having 3 slightly different caches, now only one cache is used for everything (`highpow_outofchain_headers`). This should make the changes more simple and have a negligible overhead of a few additional `GetAncesto
...
💬 brunoerg commented on issue "Failure to run Fuzz tests when running with corpus":
(https://github.com/bitcoin/bitcoin/issues/32089#issuecomment-2734905310)
Worth adding this "false positive" information to the documentation?
(https://github.com/bitcoin/bitcoin/issues/32089#issuecomment-2734905310)
Worth adding this "false positive" information to the documentation?
🤔 murchandamus reviewed a pull request: "test: Add expected result assertions"
(https://github.com/bitcoin/bitcoin/pull/31656#pullrequestreview-2696483047)
ACK a015b7e13daacdfb6db0eada50563dec70c5afb2
(https://github.com/bitcoin/bitcoin/pull/31656#pullrequestreview-2696483047)
ACK a015b7e13daacdfb6db0eada50563dec70c5afb2
🚀 fanquake merged a pull request: "test: avoid disk space warning for non-regtest"
(https://github.com/bitcoin/bitcoin/pull/32057)
(https://github.com/bitcoin/bitcoin/pull/32057)
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002163254)
The comment still doesn't really explain why. Why is this needed (now, and not with Qt 5)? Why aren't the depends outputs already suitable for static linking? Why do we need to restrict the search paths, nothing should be looking outside of depends?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002163254)
The comment still doesn't really explain why. Why is this needed (now, and not with Qt 5)? Why aren't the depends outputs already suitable for static linking? Why do we need to restrict the search paths, nothing should be looking outside of depends?
🚀 fanquake merged a pull request: "test: switch wallet_crosschain.py to signet and drop testnet4"
(https://github.com/bitcoin/bitcoin/pull/32088)
(https://github.com/bitcoin/bitcoin/pull/32088)
💬 mzumsande commented on pull request "test: Fix intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/pull/32092#discussion_r2002175061)
it would be good to also add a comment in `disconnect_p2ps` that the wait there does not guarantee that the resources of all nodes (such as outstanding txrequests) are cleared.
(https://github.com/bitcoin/bitcoin/pull/32092#discussion_r2002175061)
it would be good to also add a comment in `disconnect_p2ps` that the wait there does not guarantee that the resources of all nodes (such as outstanding txrequests) are cleared.
💬 murchandamus commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2734990522)
> The only way for the fees to be different is if the effective_value is different, however the previous condition: `utxo.GetSelectionAmount() != utxo_pool.at(utxo_pool_index - 1).GetSelectionAmount()` already compared the effective_values.
This is not accurate. You could have two UTXOs of different output types that have the same effective value but differing fees:
E.g.:
Feerate: 10 sat/vB
UTXO_1:
type: P2WPKH,
input weight: 68 vB
amount: 10,680 sats
eff_value: 10,680 - 680 = 10,000
...
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2734990522)
> The only way for the fees to be different is if the effective_value is different, however the previous condition: `utxo.GetSelectionAmount() != utxo_pool.at(utxo_pool_index - 1).GetSelectionAmount()` already compared the effective_values.
This is not accurate. You could have two UTXOs of different output types that have the same effective value but differing fees:
E.g.:
Feerate: 10 sat/vB
UTXO_1:
type: P2WPKH,
input weight: 68 vB
amount: 10,680 sats
eff_value: 10,680 - 680 = 10,000
...
👍 darosior approved a pull request: "leveldb: pull upstream C++23 changes"
(https://github.com/bitcoin/bitcoin/pull/31766#pullrequestreview-2696569585)
ACK c8fab356171a0e283d5716647e3243c04810ac51 -- checked it's a clean subtree pull from https://github.com/bitcoin-core/leveldb-subtree/tree/bitcoin-fork
(https://github.com/bitcoin/bitcoin/pull/31766#pullrequestreview-2696569585)
ACK c8fab356171a0e283d5716647e3243c04810ac51 -- checked it's a clean subtree pull from https://github.com/bitcoin-core/leveldb-subtree/tree/bitcoin-fork
💬 jimhashhq commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2735072960)
Thank you @ryanofsky, I am in favor of a multiprocess Bitcoin, I find this a well considered change. I am interested to try the updated #19461, thank you, and I do apologize for all of the questions.
From comments, it sounds like 19461 is closest, and provides the most functionality for the feature; would it make any sense to maybe change release order, putting 19461 (and a more complete #19460?) ahead of #31740 and #31741?
In general, RE `cmake` and external packages, how packages res
...
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2735072960)
Thank you @ryanofsky, I am in favor of a multiprocess Bitcoin, I find this a well considered change. I am interested to try the updated #19461, thank you, and I do apologize for all of the questions.
From comments, it sounds like 19461 is closest, and provides the most functionality for the feature; would it make any sense to maybe change release order, putting 19461 (and a more complete #19460?) ahead of #31740 and #31741?
In general, RE `cmake` and external packages, how packages res
...
⚠️ hanis12345 opened an issue: "<!--e57a25ab6845829454e8d69fc972939a-->"
(https://github.com/bitcoin/bitcoin/issues/32094)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
### Code Coverage & Benchmarks
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31766.
<!--021abf342d371248e50ceaed478a90ca-->
### Reviews
See [the guideline](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review) for information on the review process.
|
...
(https://github.com/bitcoin/bitcoin/issues/32094)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
### Code Coverage & Benchmarks
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31766.
<!--021abf342d371248e50ceaed478a90ca-->
### Reviews
See [the guideline](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review) for information on the review process.
|
...
✅ achow101 closed an issue: "."
(https://github.com/bitcoin/bitcoin/issues/32094)
(https://github.com/bitcoin/bitcoin/issues/32094)
:lock: fanquake locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/32094)
(https://github.com/bitcoin/bitcoin/issues/32094)
💬 Nouridin commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2735237660)
https://tenor.com/view/dirty-docks-shawty-triflin-shawty-triflin-she-gif-22455514
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2735237660)
https://tenor.com/view/dirty-docks-shawty-triflin-shawty-triflin-she-gif-22455514
💬 jsarenik commented on issue "Migrate from BTC/kvB to sat/vB on RPC and startup options":
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-2735442040)
What about making sat/vB fractional? I.e. `1.001` would make sure (by rounding up the absolute sats fee) there is at least 1 sat more compared to any other transaction paying just the minimal fee.
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-2735442040)
What about making sat/vB fractional? I.e. `1.001` would make sure (by rounding up the absolute sats fee) there is at least 1 sat more compared to any other transaction paying just the minimal fee.
💬 0xB10C commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#issuecomment-2735498232)
had a quick look, lgtm ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
(https://github.com/bitcoin/bitcoin/pull/32091#issuecomment-2735498232)
had a quick look, lgtm ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002562814)
> Why is this needed (now, and not with Qt 5)?
Qt 6 has a completely different build system from Qt 5.
> Why aren't the depends outputs already suitable for static linking?
Not "depends output" but rather pkg-config output.
> Why do we need to restrict the search paths, nothing should be looking outside of depends?
Because that is the goal of depends, isn't it?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2002562814)
> Why is this needed (now, and not with Qt 5)?
Qt 6 has a completely different build system from Qt 5.
> Why aren't the depends outputs already suitable for static linking?
Not "depends output" but rather pkg-config output.
> Why do we need to restrict the search paths, nothing should be looking outside of depends?
Because that is the goal of depends, isn't it?