💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2705005806)
Moved 49f155efbfb65ab60c7c67597f68489893015c71 before the chainparams updates so the commit-by-commit CI passes.
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2705005806)
Moved 49f155efbfb65ab60c7c67597f68489893015c71 before the chainparams updates so the commit-by-commit CI passes.
💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984093001)
Already repushed, with no change apart from commit order for the CI (thanks!)
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984093001)
Already repushed, with no change apart from commit order for the CI (thanks!)
💬 l0rinc commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2705008756)
I have mostly reviewed the regexes, for the rest it's only a very lightweight ACK f0b659716bd455dca02053df573d888b5a115aa4
I see you've added new values to `23`,`26`,`27`,`28` as well in `makeseeds: update user agent rege` - I can't validate those, but I assume you had a reason to extend them.
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2705008756)
I have mostly reviewed the regexes, for the rest it's only a very lightweight ACK f0b659716bd455dca02053df573d888b5a115aa4
I see you've added new values to `23`,`26`,`27`,`28` as well in `makeseeds: update user agent rege` - I can't validate those, but I assume you had a reason to extend them.
💬 l0rinc commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984094367)
I see, what do you think about 2)?
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984094367)
I see, what do you think about 2)?
💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2705011713)
> I see you've added new values to `23`,`26`,`27`,`28` as well in `makeseeds: update user agent rege` - I can't validate those, but I assume you had a reason to extend them.
Those are for additional releases, e.g. seen when running `git tag -n | sort -V` (thanks for reviewing).
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2705011713)
> I see you've added new values to `23`,`26`,`27`,`28` as well in `makeseeds: update user agent rege` - I can't validate those, but I assume you had a reason to extend them.
Those are for additional releases, e.g. seen when running `git tag -n | sort -V` (thanks for reviewing).
💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984096837)
Will do if I need to re-run the scripts and re-push.
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984096837)
Will do if I need to re-run the scripts and re-push.
💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984100831)
No strong opinion, might leave as-is for consistency with the existing code in that file setting vFixedSeeds for mainnet, test and testnet4.
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984100831)
No strong opinion, might leave as-is for consistency with the existing code in that file setting vFixedSeeds for mainnet, test and testnet4.
💬 mzumsande commented on pull request "validation: set BLOCK_FAILED_CHILD correctly":
(https://github.com/bitcoin/bitcoin/pull/31835#discussion_r1984065154)
I think this would be better suited in a different test file since invalidateblock is not part of `interfaces/chain.h`: While the existing test already calls `InvalidateBlock()`, this is just as a means to the end to test `findCommonAncestor`. Maybe `blockchain_tests.cpp` would be a better place?
(https://github.com/bitcoin/bitcoin/pull/31835#discussion_r1984065154)
I think this would be better suited in a different test file since invalidateblock is not part of `interfaces/chain.h`: While the existing test already calls `InvalidateBlock()`, this is just as a means to the end to test `findCommonAncestor`. Maybe `blockchain_tests.cpp` would be a better place?
💬 darosior commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1984063154)
This is a bit confusing, don't you think it's preferable to directly have another member in `BIP9Info` to convey this to the RPC command rather than (ab)using the stats' members?
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1984063154)
This is a bit confusing, don't you think it's preferable to directly have another member in `BIP9Info` to convey this to the RPC command rather than (ab)using the stats' members?
💬 darosior commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1982178935)
No need for the condition on test network and the seemingly-magic 2016 value here, you could just always return `DifficultyAdjustmentInterval()`?
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1982178935)
No need for the condition on test network and the seemingly-magic 2016 value here, you could just always return `DifficultyAdjustmentInterval()`?
🤔 darosior reviewed a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2662479079)
Concept ACK. A couple comments as i was reading through, will delve deeper soon.
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2662479079)
Concept ACK. A couple comments as i was reading through, will delve deeper soon.
💬 adamandrews1 commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1984147336)
I agree think it's a bit too important for a 4th positional arg
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1984147336)
I agree think it's a bit too important for a 4th positional arg
💬 adamandrews1 commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1984148053)
it's weird that it's signed, but I think we should use the same type as the docs
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1984148053)
it's weird that it's signed, but I think we should use the same type as the docs
💬 adamandrews1 commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2705103054)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2705103054)
Concept ACK
💬 purpleKarrot commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2705122486)
> even though the check is being re-run
No, the check is not rerun, [the result is cached](https://cmake.org/cmake/help/latest/module/CheckCXXSourceCompiles.html).
In this particular case, where `check_cxx_source_compiles` is used not to check for compiler capability, but for checking the availability of dependencies, where it is expected that users will want to rerun the check after fixing the problem, it may make sense to reset the cache entry by adding the following in `TryAppendLinkerFlag.
...
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2705122486)
> even though the check is being re-run
No, the check is not rerun, [the result is cached](https://cmake.org/cmake/help/latest/module/CheckCXXSourceCompiles.html).
In this particular case, where `check_cxx_source_compiles` is used not to check for compiler capability, but for checking the availability of dependencies, where it is expected that users will want to rerun the check after fixing the problem, it may make sense to reset the cache entry by adding the following in `TryAppendLinkerFlag.
...
📝 hodlinator opened a pull request: "qa: Fix TxIndex race conditions"
(https://github.com/bitcoin/bitcoin/pull/32010)
- Add synchronization in 3 places where if the Transaction Index happens to be slow, we get rare test failures when querying it for transactions (one such case experienced on Windows, prompting investigation).
- Remove unnecessary TxIndex initialization in some tests.
- Add some test coverage where TxIndex aspect could be tested in feature_init.py.
- Make feature_init.py run twice as fast through avoiding some node restarts.
(https://github.com/bitcoin/bitcoin/pull/32010)
- Add synchronization in 3 places where if the Transaction Index happens to be slow, we get rare test failures when querying it for transactions (one such case experienced on Windows, prompting investigation).
- Remove unnecessary TxIndex initialization in some tests.
- Add some test coverage where TxIndex aspect could be tested in feature_init.py.
- Make feature_init.py run twice as fast through avoiding some node restarts.
💬 mzumsande commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1984182173)
why unnecessary? The point of the test (which has a very different "philosophy" from most of our other functional tests) is to trigger these restarts on purpose, simulating power loss / node crashes at multiple different points during the init sequence, to make sure that indexes or chainstate do not get corrupted - this has been a source of bugs in the past.
If you do fewer of them, the test will obviously run faster, but you also reduce the scope of the test.
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1984182173)
why unnecessary? The point of the test (which has a very different "philosophy" from most of our other functional tests) is to trigger these restarts on purpose, simulating power loss / node crashes at multiple different points during the init sequence, to make sure that indexes or chainstate do not get corrupted - this has been a source of bugs in the past.
If you do fewer of them, the test will obviously run faster, but you also reduce the scope of the test.
💬 hodlinator commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1984192560)
Thanks! I was relying too much on the log lines from the test saying the node would "exit" and not looking deeper into why we were calling `sigterm_node()`.
Dropped that commit and changed the log line to say "terminate" instead of "exit".
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1984192560)
Thanks! I was relying too much on the log lines from the test saying the node would "exit" and not looking deeper into why we were calling `sigterm_node()`.
Dropped that commit and changed the log line to say "terminate" instead of "exit".
💬 mzumsande commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1984208213)
oh, the tests uses SIGTERM - so it doesn't simulate a power loss like I said above, but a user abort - but the general point still stands. (I confused it with similar tests I sometimes run but never PR'ed which use SIGKILL).
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1984208213)
oh, the tests uses SIGTERM - so it doesn't simulate a power loss like I said above, but a user abort - but the general point still stands. (I confused it with similar tests I sometimes run but never PR'ed which use SIGKILL).
📝 wgyt opened a pull request: "Docs: fix typos in documentation files"
(https://github.com/bitcoin/bitcoin/pull/32011)
(https://github.com/bitcoin/bitcoin/pull/32011)