💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3403556200)
> what's the reason for the frequent pushes here recently?
I wrote it above, https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3398073492. I'm working on a few changes that improves parallelism across indexes and the structure of the code, which work properly locally but are not stable on the CI yet. Will update the state with the modifications upon finishing.
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3403556200)
> what's the reason for the frequent pushes here recently?
I wrote it above, https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3398073492. I'm working on a few changes that improves parallelism across indexes and the structure of the code, which work properly locally but are not stable on the CI yet. Will update the state with the modifications upon finishing.
💬 waketraindev commented on pull request "rpc: add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3403571554)
Rebased on current master.
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3403571554)
Rebased on current master.
💬 hodlinator commented on pull request "http: Make server shutdown more robust":
(https://github.com/bitcoin/bitcoin/pull/31929#issuecomment-3403587613)
Spent some time revisiting and polishing this since 24558c2cf18210f46d6e2fadf0c5c5912f4b8e10.
* Rebased on latest master.
* Dropped the comment changes from 1350087f9a as verifying them is added work and I'm not sure they were 100% correct.
* Dropped the introduction of our own request ids in 24558c2cf1.
* Merged the the remaining log message on HTTP connection close from 1350087f9a + now log request pointers instead of ids in 028301e775, "http: Include request ptr in initial message and log
...
(https://github.com/bitcoin/bitcoin/pull/31929#issuecomment-3403587613)
Spent some time revisiting and polishing this since 24558c2cf18210f46d6e2fadf0c5c5912f4b8e10.
* Rebased on latest master.
* Dropped the comment changes from 1350087f9a as verifying them is added work and I'm not sure they were 100% correct.
* Dropped the introduction of our own request ids in 24558c2cf1.
* Merged the the remaining log message on HTTP connection close from 1350087f9a + now log request pointers instead of ids in 028301e775, "http: Include request ptr in initial message and log
...
🤔 glozow reviewed a pull request: "test: Fix reorg patterns in tests to use proper fork-based approach"
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-3337389726)
Looks pretty good, just a few comments.
A nice-to-have: in the functional tests with multiple nodes, it would be a bit more realistic / interesting if the two forks were mined by different nodes.
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-3337389726)
Looks pretty good, just a few comments.
A nice-to-have: in the functional tests with multiple nodes, it would be a bit more realistic / interesting if the two forks were mined by different nodes.
💬 glozow commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430392576)
nit or something to consider for the future: The style convention is to put them in alphabetical order on separate lines so that diffs are smaller/clearer when you add or remove. This change, for instance, changes a lot more lines than necessary.
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430392576)
nit or something to consider for the future: The style convention is to put them in alphabetical order on separate lines so that diffs are smaller/clearer when you add or remove. This change, for instance, changes a lot more lines than necessary.
💬 glozow commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430443357)
There is still an `invalidateblock` in this test?
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430443357)
There is still an `invalidateblock` in this test?
💬 glozow commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430441287)
Seems strange to have them both submit the fork blocks. In a real reorg, node1 will receive the blocks from node0 and should process them in the same way. We should test that that's true.
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430441287)
Seems strange to have them both submit the fork blocks. In a real reorg, node1 will receive the blocks from node0 and should process them in the same way. We should test that that's true.
💬 glozow commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430396234)
nit: these changes are in f1ede0e61129fbef82e7d7c261cdb8dd8a5c0aa1 when they should be in 0f1f02995b715a83f71b1b8fc92fa520625ec427
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430396234)
nit: these changes are in f1ede0e61129fbef82e7d7c261cdb8dd8a5c0aa1 when they should be in 0f1f02995b715a83f71b1b8fc92fa520625ec427
💬 glozow commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430420226)
Right, the children aren't from the disconnected blocks but I agree it's fine.
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430420226)
Right, the children aren't from the disconnected blocks but I agree it's fine.
💬 glozow commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430401045)
nit: This is defined in all the test files and is always 10 except in one place. I'd probably make it a default arg?
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430401045)
nit: This is defined in all the test files and is always 10 except in one place. I'd probably make it a default arg?
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3403611574)
I'm fine with doing that in a follow-up if you think it's too complicated (though it's likely quite simple, we can just track the very first cache miss and always prefetch after that - that heuristic would even survive node restarts).
But I don't yet see this PR as close to being final yet - do you? I still want to review it thoroughly, I don't think we should ossify yet :)
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3403611574)
I'm fine with doing that in a follow-up if you think it's too complicated (though it's likely quite simple, we can just track the very first cache miss and always prefetch after that - that heuristic would even survive node restarts).
But I don't yet see this PR as close to being final yet - do you? I still want to review it thoroughly, I don't think we should ossify yet :)
💬 pablomartin4btc commented on issue "Qt6 version of Bitcoin Core (bitcoin-qt) flickers on Wayland":
(https://github.com/bitcoin-core/gui/issues/903#issuecomment-3403636095)
> can we just get rid of the problematic code in this case?
I'll take a look...
(https://github.com/bitcoin-core/gui/issues/903#issuecomment-3403636095)
> can we just get rid of the problematic code in this case?
I'll take a look...
💬 l0rinc commented on pull request "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`":
(https://github.com/bitcoin/bitcoin/pull/33624#issuecomment-3403664504)
Verified with:
```patch
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 9d09872597..d43bf14b7e 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -147,7 +147,7 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i
if (tx.IsCoinBase())
return nSigOps;
- if (flags & SCRIPT_VERIFY_P2SH) {
+ if (true) {
nSigOps += GetP2SHSigOpCount(tx, inputs) * WITNESS_SCALE_FACTOR;
...
(https://github.com/bitcoin/bitcoin/pull/33624#issuecomment-3403664504)
Verified with:
```patch
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 9d09872597..d43bf14b7e 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -147,7 +147,7 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i
if (tx.IsCoinBase())
return nSigOps;
- if (flags & SCRIPT_VERIFY_P2SH) {
+ if (true) {
nSigOps += GetP2SHSigOpCount(tx, inputs) * WITNESS_SCALE_FACTOR;
...
💬 kevkevinpal commented on pull request "doc: correct topology requirements in submitpackage helptext":
(https://github.com/bitcoin/bitcoin/pull/33630#issuecomment-3403688479)
ACK [501b030](https://github.com/bitcoin/bitcoin/pull/33630/commits/501b030769ae538c5fc445729e82f0b08bcd288a)
Fairly straightforward change and makes it clearer what is accepted
(https://github.com/bitcoin/bitcoin/pull/33630#issuecomment-3403688479)
ACK [501b030](https://github.com/bitcoin/bitcoin/pull/33630/commits/501b030769ae538c5fc445729e82f0b08bcd288a)
Fairly straightforward change and makes it clearer what is accepted
📝 fjahr opened a pull request: "init: Split file path handling out of -asmap"
(https://github.com/bitcoin/bitcoin/pull/33631)
Note: There is an alternative change that leaves the behavior of `-asmap` as is but makes slight improvements and adds better documentation: XXX Please indicate with your conceptual review which option you prefer, thanks!
This change adds a new option `-asmapfile` which allows to set the asmap file, while `-asmap` now is simply a bool option. Also fixes the asmap functional test docs, since then had become out of sync with the actual content of the test.
The current handling of `-asmap` is
...
(https://github.com/bitcoin/bitcoin/pull/33631)
Note: There is an alternative change that leaves the behavior of `-asmap` as is but makes slight improvements and adds better documentation: XXX Please indicate with your conceptual review which option you prefer, thanks!
This change adds a new option `-asmapfile` which allows to set the asmap file, while `-asmap` now is simply a bool option. Also fixes the asmap functional test docs, since then had become out of sync with the actual content of the test.
The current handling of `-asmap` is
...
📝 fjahr opened a pull request: "init: Improve -asmap option behavior and documentation"
(https://github.com/bitcoin/bitcoin/pull/33632)
Note: There is an alternative change that turns `-asmap` into a bool option and adds a `-asmapfile` option: #33631 Please indicate with your conceptual review which option you prefer, thanks!
This changes the behavior of the `GetPathArg` function to better work with the behavior of `-asmap`. When a path arg is given "1" (for example -asmap=1), this would currently be interpreted as a path but should be interpreted as using the arg without a parameter which activates the fallback. There is no
...
(https://github.com/bitcoin/bitcoin/pull/33632)
Note: There is an alternative change that turns `-asmap` into a bool option and adds a `-asmapfile` option: #33631 Please indicate with your conceptual review which option you prefer, thanks!
This changes the behavior of the `GetPathArg` function to better work with the behavior of `-asmap`. When a path arg is given "1" (for example -asmap=1), this would currently be interpreted as a path but should be interpreted as using the arg without a parameter which activates the fallback. There is no
...
💬 fjahr commented on issue "Default ASmap file path is not used unless -asmap is set":
(https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3403824021)
> As ryanofsky said, this seems to imply that the default value is used when the option is not specified.
Thanks for clarifying @vostrnad @ryanofsky , I agree that this part is confusing and I didn't see that way of reading it.
Initially I had a preference to not change the behavior of `-asmap` and introduce a separate option for the file path, but recently this has grown on me. So I have opened two alternative PRs now, one which leaves the option unchanged but improves the `=1` case and adds
...
(https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3403824021)
> As ryanofsky said, this seems to imply that the default value is used when the option is not specified.
Thanks for clarifying @vostrnad @ryanofsky , I agree that this part is confusing and I didn't see that way of reading it.
Initially I had a preference to not change the behavior of `-asmap` and introduce a separate option for the file path, but recently this has grown on me. So I have opened two alternative PRs now, one which leaves the option unchanged but improves the `=1` case and adds
...
💬 Sammie05 commented on pull request "doc: correct topology requirements in submitpackage helptext":
(https://github.com/bitcoin/bitcoin/pull/33630#issuecomment-3403847243)
This update actually mproves the clarity of the package requirements by explicitly stating the conditions under which transactions can be included. It helps ensure users understand that not all parents need to be present, while maintaining the necessity for topological sorting. Great job on enhancing the documentation!
(https://github.com/bitcoin/bitcoin/pull/33630#issuecomment-3403847243)
This update actually mproves the clarity of the package requirements by explicitly stating the conditions under which transactions can be included. It helps ensure users understand that not all parents need to be present, while maintaining the necessity for topological sorting. Great job on enhancing the documentation!
💬 yancyribbens commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2430736768)
No worries I can remove it then.
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2430736768)
No worries I can remove it then.
💬 yancyribbens commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2430748617)
Hmm it's probably silly to have it both places. BnB for example does not have it's params defined in `.h` at all, nor its description for that matter. And the params in `.h` for SRD are missing `change_fee` :face_palm:.
Honestly, all these comments should be in the header file, right? It would be really hard to get people to review that kind of a change though.
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2430748617)
Hmm it's probably silly to have it both places. BnB for example does not have it's params defined in `.h` at all, nor its description for that matter. And the params in `.h` for SRD are missing `change_fee` :face_palm:.
Honestly, all these comments should be in the header file, right? It would be really hard to get people to review that kind of a change though.