💬 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)
🤔 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
...