💬 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.
💬 maflcko commented on pull request "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`":
(https://github.com/bitcoin/bitcoin/pull/33624#issuecomment-3405047054)
re-lgtm ACK 3a10d700bc1889b3690097efc935c5a4ba5966bb
Only change seems to be a comment/doc.
(https://github.com/bitcoin/bitcoin/pull/33624#issuecomment-3405047054)
re-lgtm ACK 3a10d700bc1889b3690097efc935c5a4ba5966bb
Only change seems to be a comment/doc.
🤔 janb84 reviewed a pull request: "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`"
(https://github.com/bitcoin/bitcoin/pull/33624#pullrequestreview-3338930586)
tested ACK 3a10d700bc1889b3690097efc935c5a4ba5966bb
PR introduces new assert in test that prohibits the `if ( 1 == 1)` mutation / `if(true)`, because the assert will fail in that condition.
- code review ✅
- build & tested ✅
- changed code to true to test assert ✅
(https://github.com/bitcoin/bitcoin/pull/33624#pullrequestreview-3338930586)
tested ACK 3a10d700bc1889b3690097efc935c5a4ba5966bb
PR introduces new assert in test that prohibits the `if ( 1 == 1)` mutation / `if(true)`, because the assert will fail in that condition.
- code review ✅
- build & tested ✅
- changed code to true to test assert ✅
🤔 maflcko reviewed a pull request: "init: Improve -asmap option behavior and documentation"
(https://github.com/bitcoin/bitcoin/pull/33632#pullrequestreview-3338942228)
(nit from the llm bot)
(https://github.com/bitcoin/bitcoin/pull/33632#pullrequestreview-3338942228)
(nit from the llm bot)
💬 maflcko commented on pull request "init: Improve -asmap option behavior and documentation":
(https://github.com/bitcoin/bitcoin/pull/33632#discussion_r2431523908)
Specify asn mapping used for bucketing of the peers -> Specify the ASN mapping used for bucketing of peers [adds missing article and capitalizes the ASN acronym to improve clarity]
(https://github.com/bitcoin/bitcoin/pull/33632#discussion_r2431523908)
Specify asn mapping used for bucketing of the peers -> Specify the ASN mapping used for bucketing of peers [adds missing article and capitalizes the ASN acronym to improve clarity]