💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2248990387)
Rebased fba04d7756c77ed9371c5dc90d3bebc9aa767f4b -> ec5f45b9186184cac9c64ed08b8b8cc596e301a3 ([`pr/mine.7`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.7) -> [`pr/mine.8`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.7-rebase..pr/mine.8)), moving most code that was here into base PRs #30509 and #30510. I also followed up various review suggestions above like fixing the top-level make target and dropping unneeded
...
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2248990387)
Rebased fba04d7756c77ed9371c5dc90d3bebc9aa767f4b -> ec5f45b9186184cac9c64ed08b8b8cc596e301a3 ([`pr/mine.7`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.7) -> [`pr/mine.8`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.7-rebase..pr/mine.8)), moving most code that was here into base PRs #30509 and #30510. I also followed up various review suggestions above like fixing the top-level make target and dropping unneeded
...
👍 theStack approved a pull request: "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending"
(https://github.com/bitcoin/bitcoin/pull/30352#pullrequestreview-2197876025)
Code-review ACK 67c3537f75bcf085bb98e3830649e93da124cb06
Left a non-blocking test nit. In the future a dedicated MiniWallet-mode for P2A could probably make sense (should be trivial to implement).
(https://github.com/bitcoin/bitcoin/pull/30352#pullrequestreview-2197876025)
Code-review ACK 67c3537f75bcf085bb98e3830649e93da124cb06
Left a non-blocking test nit. In the future a dedicated MiniWallet-mode for P2A could probably make sense (should be trivial to implement).
💬 theStack commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1690530356)
obviously way out of scope for this PR, but I wonder: could there be scenarios where it's useful/needed to have "signing support" for P2A outputs, by doing nothing and just succeed here? (probably not, as `createrawtransaction` with the P2A input is already sufficient to spend it).
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1690530356)
obviously way out of scope for this PR, but I wonder: could there be scenarios where it's useful/needed to have "signing support" for P2A outputs, by doing nothing and just succeed here? (probably not, as `createrawtransaction` with the P2A input is already sufficient to spend it).
💬 theStack commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1690520020)
nit, could alternatively use MiniWallet's `.send_to` method for less code (assuming that specifying nSequence explicitly is not important), e.g.:
```diff
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
index cf3185271c..4b9ff3d138 100755
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py
@@ -391,16 +391,14 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
)
self.log.info('OP_1 <0x4e73> is able to b
...
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1690520020)
nit, could alternatively use MiniWallet's `.send_to` method for less code (assuming that specifying nSequence explicitly is not important), e.g.:
```diff
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
index cf3185271c..4b9ff3d138 100755
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py
@@ -391,16 +391,14 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
)
self.log.info('OP_1 <0x4e73> is able to b
...
📝 Vick145 opened a pull request: "Create Wap"
(https://github.com/bitcoin/bitcoin/pull/30521)
http://waptrick.com
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements o
...
(https://github.com/bitcoin/bitcoin/pull/30521)
http://waptrick.com
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements o
...
💬 Vick145 commented on pull request "Create Wap":
(https://github.com/bitcoin/bitcoin/pull/30521#issuecomment-2249123371)
http://waptrick.com
(https://github.com/bitcoin/bitcoin/pull/30521#issuecomment-2249123371)
http://waptrick.com
✅ pinheadmz closed a pull request: "Create Wap"
(https://github.com/bitcoin/bitcoin/pull/30521)
(https://github.com/bitcoin/bitcoin/pull/30521)
🤔 tdb3 reviewed a pull request: "rpc: add utxo's blockhash and number of confirmations to scantxoutset output"
(https://github.com/bitcoin/bitcoin/pull/30515#pullrequestreview-2197995886)
Approach ACK
Nice addition. Seems to work well.
Ran a manual test where 102 blocks were generated, a spend to bcrt1qrm87u5l6f455x5lk5f9lmtxahdf39zrk22m8k5 was done, and two more blocks were generated. The scan height was 104, the height of the UTXO was 103, and it's reported that there are two confirmations.
```
$ src/bitcoin-cli scantxoutset start '["addr(bcrt1qrm87u5l6f455x5lk5f9lmtxahdf39zrk22m8k5)"]'
{
"success": true,
"txouts": 105,
"height": 104,
"bestblock": "4534
...
(https://github.com/bitcoin/bitcoin/pull/30515#pullrequestreview-2197995886)
Approach ACK
Nice addition. Seems to work well.
Ran a manual test where 102 blocks were generated, a spend to bcrt1qrm87u5l6f455x5lk5f9lmtxahdf39zrk22m8k5 was done, and two more blocks were generated. The scan height was 104, the height of the UTXO was 103, and it's reported that there are two confirmations.
```
$ src/bitcoin-cli scantxoutset start '["addr(bcrt1qrm87u5l6f455x5lk5f9lmtxahdf39zrk22m8k5)"]'
{
"success": true,
"txouts": 105,
"height": 104,
"bestblock": "4534
...
💬 tdb3 commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690604737)
nitty nit: If you happen to change this file again, could adjust the wording to `The block height at which the scan was done`
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690604737)
nitty nit: If you happen to change this file again, could adjust the wording to `The block height at which the scan was done`
💬 tdb3 commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690677294)
It would be great to add tests for these in `test/functional/rpc_scantxoutset.py`.
Maybe something like:
```diff
diff --git a/test/functional/rpc_scantxoutset.py b/test/functional/rpc_scantxoutset.py
index 078a24d577..8edc6d265a 100755
--- a/test/functional/rpc_scantxoutset.py
+++ b/test/functional/rpc_scantxoutset.py
@@ -120,6 +120,11 @@ class ScantxoutsetTest(BitcoinTestFramework):
assert_equal(self.nodes[0].scantxoutset("status"), None)
assert_equal(self.nodes[0].s
...
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690677294)
It would be great to add tests for these in `test/functional/rpc_scantxoutset.py`.
Maybe something like:
```diff
diff --git a/test/functional/rpc_scantxoutset.py b/test/functional/rpc_scantxoutset.py
index 078a24d577..8edc6d265a 100755
--- a/test/functional/rpc_scantxoutset.py
+++ b/test/functional/rpc_scantxoutset.py
@@ -120,6 +120,11 @@ class ScantxoutsetTest(BitcoinTestFramework):
assert_equal(self.nodes[0].scantxoutset("status"), None)
assert_equal(self.nodes[0].s
...
💬 tdb3 commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690615620)
nit: If the file is touched again.
```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index e964a65a05..5df8641f6d 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -2275,7 +2275,7 @@ static RPCHelpMan scantxoutset()
int64_t count = 0;
std::unique_ptr<CCoinsViewCursor> pcursor;
const CBlockIndex* tip;
- CChain* active_chain = nullptr;
+ CChain* active_chain{nullptr};
NodeContext& node = EnsureAny
...
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690615620)
nit: If the file is touched again.
```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index e964a65a05..5df8641f6d 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -2275,7 +2275,7 @@ static RPCHelpMan scantxoutset()
int64_t count = 0;
std::unique_ptr<CCoinsViewCursor> pcursor;
const CBlockIndex* tip;
- CChain* active_chain = nullptr;
+ CChain* active_chain{nullptr};
NodeContext& node = EnsureAny
...
💬 luisschwab commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#issuecomment-2249255752)
Fixed nits and added the test
(https://github.com/bitcoin/bitcoin/pull/30515#issuecomment-2249255752)
Fixed nits and added the test
💬 sipa commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690720835)
I don't think this is safe. When this scope closes, `cs_main` is released, and after that point, `active_chain` points to `CChain` which may be concurrently modified by another thread (which on itself would be undefined behavior already). Worse, it's possible that a reorg occurs (however unlikely) which shortens the chain, and at that point, the `(*active_chain)[coin.nHeight]` call below would be accessing the `CChain::vChain` vector out of bounds.
But I don't think you need to. `tip` already
...
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690720835)
I don't think this is safe. When this scope closes, `cs_main` is released, and after that point, `active_chain` points to `CChain` which may be concurrently modified by another thread (which on itself would be undefined behavior already). Worse, it's possible that a reorg occurs (however unlikely) which shortens the chain, and at that point, the `(*active_chain)[coin.nHeight]` call below would be accessing the `CChain::vChain` vector out of bounds.
But I don't think you need to. `tip` already
...
💬 andrewtoth commented on pull request "optimization: Precalculate SipHash constant XOR with k0 and k1 in SaltedOutpointHasher":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-2249287267)
Based on your benchmark results, I don't think this can be called an optimization. It seems to be worse for one benchmark, better in another, and roughly the same for the rest. The description also notes that this will not be noticeable by users. In light of that, does the motivation for this PR still hold?
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-2249287267)
Based on your benchmark results, I don't think this can be called an optimization. It seems to be worse for one benchmark, better in another, and roughly the same for the rest. The description also notes that this will not be noticeable by users. In light of that, does the motivation for this PR still hold?
💬 luisschwab commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690734288)
You're right, it is unsafe. Your solution is a lot cleaner as well.
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690734288)
You're right, it is unsafe. Your solution is a lot cleaner as well.
💬 fjahr commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2249392193)
> Can you clarify what you mean with "not quite"? Locally, if I compile with undefined,integer,float-divide-by-zero and run my diff with: UBSAN_OPTIONS="suppressions=$PWD/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./test/functional/feature_assumeutxo.py, I get the same crash.
Ah, it wasn't clear to me that the I would have to run it with the sanitizer. That's why it didn't fail for me.
You can reproduce the issue and make the test fail with t
...
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2249392193)
> Can you clarify what you mean with "not quite"? Locally, if I compile with undefined,integer,float-divide-by-zero and run my diff with: UBSAN_OPTIONS="suppressions=$PWD/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./test/functional/feature_assumeutxo.py, I get the same crash.
Ah, it wasn't clear to me that the I would have to run it with the sanitizer. That's why it didn't fail for me.
You can reproduce the issue and make the test fail with t
...
💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690825056)
> while we're at it, should I also remove this cast?
Sure, the cast is completely harmless (because a value high enough for the sign or value to change can't exist), but the cast is confusing at best, so best to remove.
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690825056)
> while we're at it, should I also remove this cast?
Sure, the cast is completely harmless (because a value high enough for the sign or value to change can't exist), but the cast is confusing at best, so best to remove.
💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#issuecomment-2249457002)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/30515#issuecomment-2249457002)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690827566)
nit: The two fields should have the same wording, as they refer to the same block
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690827566)
nit: The two fields should have the same wording, as they refer to the same block
💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690834408)
```suggestion
{RPCResult::Type::NUM, "confirmations", "Number of confirmations of the unspent transaction output when the scan was done"},
```
nit: (Same wording here, because the `confirmations` refers in the calculation to the same block)
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690834408)
```suggestion
{RPCResult::Type::NUM, "confirmations", "Number of confirmations of the unspent transaction output when the scan was done"},
```
nit: (Same wording here, because the `confirmations` refers in the calculation to the same block)
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690844876)
> ```diff
> + COutPoint out{*txid, static_cast<uint32_t>(nOut)};
> ```
Thanks for looking again. For reference, `getInt` has a built in check, so you could use `getInt<uint32_t>` and avoid this cast. That'd be a behavior change for out-of-range values, but the values are out of range anyway, so should be fine.
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690844876)
> ```diff
> + COutPoint out{*txid, static_cast<uint32_t>(nOut)};
> ```
Thanks for looking again. For reference, `getInt` has a built in check, so you could use `getInt<uint32_t>` and avoid this cast. That'd be a behavior change for out-of-range values, but the values are out of range anyway, so should be fine.