📝 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)
🤔 jonatack reviewed a pull request: "Docs: fix typos in documentation files"
(https://github.com/bitcoin/bitcoin/pull/32011#pullrequestreview-2666130196)
ACK 01c14b3511610c5d5c6f509aadee1836220753ac modulo failing CI (see comment below)
(https://github.com/bitcoin/bitcoin/pull/32011#pullrequestreview-2666130196)
ACK 01c14b3511610c5d5c6f509aadee1836220753ac modulo failing CI (see comment below)
💬 jonatack commented on pull request "Docs: fix typos in documentation files":
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984377258)
This change is on a subtree directory and would need to be made upstream, not here. You can verify this locally by running `./test/lint/git-subtree-check.sh src/leveldb` from repo root.
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984377258)
This change is on a subtree directory and would need to be made upstream, not here. You can verify this locally by running `./test/lint/git-subtree-check.sh src/leveldb` from repo root.
💬 wgyt commented on pull request "Docs: fix typos in documentation files":
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984387492)
Thanks for sharing this, I have reverted this change.
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984387492)
Thanks for sharing this, I have reverted this change.
🤔 jonatack reviewed a pull request: "Remove Taproot activation height"
(https://github.com/bitcoin/bitcoin/pull/26201#pullrequestreview-2666162948)
Light approach ACK 301e41985ef8cc8e3098d1d737864a5ae1f5108e
(https://github.com/bitcoin/bitcoin/pull/26201#pullrequestreview-2666162948)
Light approach ACK 301e41985ef8cc8e3098d1d737864a5ae1f5108e
💬 jonatack commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1984399781)
Should this be like https://github.com/bitcoin/bitcoin/pull/26201/files#diff-decae4be02fb8a47ab4557fe74a9cb853bdfa3ec0fa1b515c0a1e5de91f4ad0bR1418?
```suggestion
* Consensus changes for which the new rules are enforced from genesis are not listed here.
```
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1984399781)
Should this be like https://github.com/bitcoin/bitcoin/pull/26201/files#diff-decae4be02fb8a47ab4557fe74a9cb853bdfa3ec0fa1b515c0a1e5de91f4ad0bR1418?
```suggestion
* Consensus changes for which the new rules are enforced from genesis are not listed here.
```
💬 jonatack commented on pull request "Docs: fix typos in documentation files":
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984404706)
You'll need to squash the fixup commit into the first one (see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits).
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984404706)
You'll need to squash the fixup commit into the first one (see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits).
💬 wgyt commented on pull request "Docs: fix typos in documentation files":
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984410203)
OK, I have squashed the commit. :)
(https://github.com/bitcoin/bitcoin/pull/32011#discussion_r1984410203)
OK, I have squashed the commit. :)
💬 tnndbtc commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1984419437)
Agree, added.
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1984419437)
Agree, added.
💬 tnndbtc commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1984420241)
Agree both. Provided comment on why adding the loop to generate number of ZEROs for nsat_return. Also, squashed multiple commits into one commit.
(https://github.com/bitcoin/bitcoin/pull/31719#discussion_r1984420241)
Agree both. Provided comment on why adding the loop to generate number of ZEROs for nsat_return. Also, squashed multiple commits into one commit.
⚠️ jirijakes opened an issue: "test: No unit test covers BIP342 tapscript signatures"
(https://github.com/bitcoin/bitcoin/issues/32012)
The following change does not make any unit test fail:
```diff
diff --git i/src/script/interpreter.cpp w/src/script/interpreter.cpp
index a35306b693..d9c119ae77 100644
--- i/src/script/interpreter.cpp
+++ w/src/script/interpreter.cpp
@@ -1484,7 +1484,7 @@ bool SignatureHashSchnorr(uint256& hash_out, ScriptExecutionData& execdata, cons
// key_version is not used and left uninitialized.
break;
case SigVersion::TAPSCRIPT:
- ext_flag = 1;
+ ext_flag = 42;
...
(https://github.com/bitcoin/bitcoin/issues/32012)
The following change does not make any unit test fail:
```diff
diff --git i/src/script/interpreter.cpp w/src/script/interpreter.cpp
index a35306b693..d9c119ae77 100644
--- i/src/script/interpreter.cpp
+++ w/src/script/interpreter.cpp
@@ -1484,7 +1484,7 @@ bool SignatureHashSchnorr(uint256& hash_out, ScriptExecutionData& execdata, cons
// key_version is not used and left uninitialized.
break;
case SigVersion::TAPSCRIPT:
- ext_flag = 1;
+ ext_flag = 42;
...
💬 tnndbtc commented on pull request "miniscript: fixes #29098 by only use first k valid signatures":
(https://github.com/bitcoin/bitcoin/pull/31719#issuecomment-2705493881)
@hodlinator Appreciate your valuable comments and reviews. I've made the changes accordingly so you can check them out one by one.
As for the choice "_I think it would be good if this PR did one of either:
a) Directly included https://github.com/bitcoin/bitcoin/commit/387c12e0813a863bc9728777719acbafe7b12a34 from https://github.com/bitcoin/bitcoin/pull/28212, or
b) Not mention https://github.com/bitcoin/bitcoin/commit/387c12e0813a863bc9728777719acbafe7b12a34/https://github.com/bitcoin/bitc
...
(https://github.com/bitcoin/bitcoin/pull/31719#issuecomment-2705493881)
@hodlinator Appreciate your valuable comments and reviews. I've made the changes accordingly so you can check them out one by one.
As for the choice "_I think it would be good if this PR did one of either:
a) Directly included https://github.com/bitcoin/bitcoin/commit/387c12e0813a863bc9728777719acbafe7b12a34 from https://github.com/bitcoin/bitcoin/pull/28212, or
b) Not mention https://github.com/bitcoin/bitcoin/commit/387c12e0813a863bc9728777719acbafe7b12a34/https://github.com/bitcoin/bitc
...
💬 hebasto commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2705675642)
> > 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 `TryAppendL
...
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2705675642)
> > 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 `TryAppendL
...
💬 hebasto commented on pull request "RFC contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2705678071)
Concept ACK for the same reasoning as in https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2704999209.
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2705678071)
Concept ACK for the same reasoning as in https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2704999209.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2705689094)
> Unsure if a release note was written, apologies if I missed it. @hebasto @theuni @fanquake could you add to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft?
Added.
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2705689094)
> Unsure if a release note was written, apologies if I missed it. @hebasto @theuni @fanquake could you add to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft?
Added.
🤔 i-am-yuvi reviewed a pull request: "qa: clarify and document one assumeutxo test case with malleated snapshot"
(https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2666431885)
tACK e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65
- Ran all functional and unit test for assume utxo
- Tested with different invalid amounts
- Verified the serialization
(https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2666431885)
tACK e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65
- Ran all functional and unit test for assume utxo
- Tested with different invalid amounts
- Verified the serialization