💬 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.
💬 PlanetMacro commented on issue "Revert OP_RETURN Policy Changes in Bitcoin Core v0.30":
(https://github.com/bitcoin/bitcoin/issues/33619#issuecomment-3403999866)
Spam attacks are issues
(https://github.com/bitcoin/bitcoin/issues/33619#issuecomment-3403999866)
Spam attacks are issues
💬 yancyribbens commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#issuecomment-3404018085)
@brunoerg thanks for reviewing. Instead of adding new docs I improved the comments in `.h` instead. If only the other selection algorithms did that as well..
(https://github.com/bitcoin/bitcoin/pull/33622#issuecomment-3404018085)
@brunoerg thanks for reviewing. Instead of adding new docs I improved the comments in `.h` instead. If only the other selection algorithms did that as well..
💬 furszy commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r2430843261)
This can be closed. I pushed it on https://github.com/bitcoin/bitcoin/pull/32694.
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r2430843261)
This can be closed. I pushed it on https://github.com/bitcoin/bitcoin/pull/32694.
💬 Sammie05 commented on pull request "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`":
(https://github.com/bitcoin/bitcoin/pull/33624#issuecomment-3404152930)
This test case ensures that sigopcount_tests.cpp effectively checks that P2SH sigops aren't counted when the verification flag is absent. This helps ensure transaction validation remains accurate and reliable.
(https://github.com/bitcoin/bitcoin/pull/33624#issuecomment-3404152930)
This test case ensures that sigopcount_tests.cpp effectively checks that P2SH sigops aren't counted when the verification flag is absent. This helps ensure transaction validation remains accurate and reliable.
💬 Sammie05 commented on pull request "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`":
(https://github.com/bitcoin/bitcoin/pull/33624#issuecomment-3404156556)
> This test case ensures that sigopcount_tests.cpp effectively checks that P2SH sigops aren't counted when the verification flag is absent. which makes transaction validation remains accurate and reliable.
(https://github.com/bitcoin/bitcoin/pull/33624#issuecomment-3404156556)
> This test case ensures that sigopcount_tests.cpp effectively checks that P2SH sigops aren't counted when the verification flag is absent. which makes transaction validation remains accurate and reliable.
💬 l0rinc commented on pull request "leveldb: show non-default options during init":
(https://github.com/bitcoin/bitcoin/pull/31644#issuecomment-3404295703)
Thanks for the hint @pablomartin4btc, I kept the original line and added and extra debug log, when starting with `-debug=leveldb`.
(https://github.com/bitcoin/bitcoin/pull/31644#issuecomment-3404295703)
Thanks for the hint @pablomartin4btc, I kept the original line and added and extra debug log, when starting with `-debug=leveldb`.
💬 maflcko commented on pull request "ci: run native fuzz with MSAN job":
(https://github.com/bitcoin/bitcoin/pull/33626#issuecomment-3404776272)
No objection, but my worry is that the error is not exactly trivial to understand and actionable:
* In https://github.com/bitcoin/bitcoin/commit/9cb9651d92ddb5d92724f6a52440601c7a0bbcf8 it was fixed by changing the linker. While the patch looks harmless and is easy to test to fix the issue, I don't think it is straightforward to see why a runtime sanitizer issue is addressed by link-time changes.
* In https://github.com/bitcoin/bitcoin/pull/29676#issuecomment-2007701379 it was fixed by accid
...
(https://github.com/bitcoin/bitcoin/pull/33626#issuecomment-3404776272)
No objection, but my worry is that the error is not exactly trivial to understand and actionable:
* In https://github.com/bitcoin/bitcoin/commit/9cb9651d92ddb5d92724f6a52440601c7a0bbcf8 it was fixed by changing the linker. While the patch looks harmless and is easy to test to fix the issue, I don't think it is straightforward to see why a runtime sanitizer issue is addressed by link-time changes.
* In https://github.com/bitcoin/bitcoin/pull/29676#issuecomment-2007701379 it was fixed by accid
...
💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3404864601)
Thank you @glozow and @instagibbs for the review.
Addressed feedback:
- Removed the `create_empty_fork` method per @instagibbs's [comment](https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3402220995)
- Modified changes in the next commit as suggested by @glozow ([comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430396234))
- Made `FORK_LENGTH` a default argument per @glozow's [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430401045)
- Rem
...
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3404864601)
Thank you @glozow and @instagibbs for the review.
Addressed feedback:
- Removed the `create_empty_fork` method per @instagibbs's [comment](https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3402220995)
- Modified changes in the next commit as suggested by @glozow ([comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430396234))
- Made `FORK_LENGTH` a default argument per @glozow's [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430401045)
- Rem
...
💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#issuecomment-3404866372)
`1197919eab...07a926474b`: address some suggestions
(https://github.com/bitcoin/bitcoin/pull/33567#issuecomment-3404866372)
`1197919eab...07a926474b`: address some suggestions
💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431349116)
Done.
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431349116)
Done.
💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431352462)
Done. Also shortened the names a little bit because now values have to be prefixed by the enum name.
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431352462)
Done. Also shortened the names a little bit because now values have to be prefixed by the enum name.
💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431356881)
It isn't a C++ method, but is a [method](https://www.merriam-webster.com/dictionary/method), "a way, technique, or process of or for doing something".
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431356881)
It isn't a C++ method, but is a [method](https://www.merriam-webster.com/dictionary/method), "a way, technique, or process of or for doing something".
💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431362561)
To make sure that if a new enum value is added it will be handled properly - this snippet will be updated. If it is forgotten, then there will be a compiler warning about unhandled enum value.
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431362561)
To make sure that if a new enum value is added it will be handled properly - this snippet will be updated. If it is forgotten, then there will be a compiler warning about unhandled enum value.
💬 vasild commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431365052)
I prefer to avoid duplicating `WalletLogPrintf("Submitting wtx...", wtx.GetHash().ToString());`
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2431365052)
I prefer to avoid duplicating `WalletLogPrintf("Submitting wtx...", wtx.GetHash().ToString());`
🤔 stratospher reviewed a pull request: "test: add functional test for `TestShell` (matching doc example)"
(https://github.com/bitcoin/bitcoin/pull/33546#pullrequestreview-3338732396)
ACK 57f7c68.
(https://github.com/bitcoin/bitcoin/pull/33546#pullrequestreview-3338732396)
ACK 57f7c68.
💬 stratospher commented on pull request "test: add functional test for `TestShell` (matching doc example)":
(https://github.com/bitcoin/bitcoin/pull/33546#discussion_r2431376770)
57f7c68: nit: would keeping it in something like `TEST_FRAMEWORK_TESTSHELL_TESTS` (similar to `TEST_FRAMEWORK_UNIT_TESTS` be a better place) since it's a test framework infra test.
(https://github.com/bitcoin/bitcoin/pull/33546#discussion_r2431376770)
57f7c68: nit: would keeping it in something like `TEST_FRAMEWORK_TESTSHELL_TESTS` (similar to `TEST_FRAMEWORK_UNIT_TESTS` be a better place) since it's a test framework infra test.