💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385744)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385744)
Done.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386555)
I've added comments explaining this, and also moved the "test correspondence between Cluster and DepGraph" to a new `VerifyDepGraphFromCluster` function that's invoked in both the unit and fuzz tests.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386555)
I've added comments explaining this, and also moved the "test correspondence between Cluster and DepGraph" to a new `VerifyDepGraphFromCluster` function that's invoked in both the unit and fuzz tests.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386667)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386667)
Done.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2248844497)
added fuzz and unit tests which cover more than the functional tests can alone
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2248844497)
added fuzz and unit tests which cover more than the functional tests can alone
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386739)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386739)
Done.
💬 instagibbs commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#issuecomment-2248854763)
reACK https://github.com/bitcoin/bitcoin/pull/30275/commits/25bf86a225b0df3f48ade1016b47f5ee1636b988
tests make me feel much better about this thanks
(https://github.com/bitcoin/bitcoin/pull/30275#issuecomment-2248854763)
reACK https://github.com/bitcoin/bitcoin/pull/30275/commits/25bf86a225b0df3f48ade1016b47f5ee1636b988
tests make me feel much better about this thanks
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2248939831)
Was able to simplify this by dropping CustomBuildMessage/CustomReadMessage functions and tests. It now requires the latest version of the libmultiprocess library though.
Rebased 79cfc0d198faecec598f923e6bf83ccf8dc09bbb -> 6cccb436557c58e0ccd21ffe0eaf31058f5cb799 ([`pr/mine-types.2`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.2) -> [`pr/mine-types.3`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-ty
...
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2248939831)
Was able to simplify this by dropping CustomBuildMessage/CustomReadMessage functions and tests. It now requires the latest version of the libmultiprocess library though.
Rebased 79cfc0d198faecec598f923e6bf83ccf8dc09bbb -> 6cccb436557c58e0ccd21ffe0eaf31058f5cb799 ([`pr/mine-types.2`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.2) -> [`pr/mine-types.3`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-ty
...
💬 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?