💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472586945)
Ah makes sense! I think it would be good to add a test for this case?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472586945)
Ah makes sense! I think it would be good to add a test for this case?
💬 maflcko commented on pull request "assumeutxo, rpc: Add 'start' parameter to loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1918815388)
Ok, maybe turn it into draft, until you are done and the CI is passing?
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1918815388)
Ok, maybe turn it into draft, until you are done and the CI is passing?
🤔 fanquake reviewed a pull request: "init: settings, do not load auto-generated warning msg"
(https://github.com/bitcoin/bitcoin/pull/29301#pullrequestreview-1853403738)
Seems fine. Gets rid of the pointless `2024-01-31T10:25:34Z Ignoring unknown rw_settings value _warning_` and
`2024-01-31T10:25:34Z Setting file arg: _warning_ = "This file is automatically generated and updated by Bitcoin Core. Please do not edit this file while the node is running, as any changes might be ignored or overwritten."` output.
(https://github.com/bitcoin/bitcoin/pull/29301#pullrequestreview-1853403738)
Seems fine. Gets rid of the pointless `2024-01-31T10:25:34Z Ignoring unknown rw_settings value _warning_` and
`2024-01-31T10:25:34Z Setting file arg: _warning_ = "This file is automatically generated and updated by Bitcoin Core. Please do not edit this file while the node is running, as any changes might be ignored or overwritten."` output.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1472632779)
yeah not sigops adjusted, but we don't expect that to be necessary here so I think it's fine with a comment
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1472632779)
yeah not sigops adjusted, but we don't expect that to be necessary here so I think it's fine with a comment
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472650551)
Yep! There are two ways to use this data:
1. Immediately, as `(b_scan * input_hash) * sum_input_pubkeys`. This is one scalar multiplication and one EC multiplication.
2. Store it for later (either for wallet re-scans or to serve to light clients), as `input_hash * sum_input_pubkeys`. This is one EC multiplication and allows us to store all of the input data needed as 33 bytes.
If the function instead returned `input_hash * sum_input_pubkeys`, we would end up doing two EC multiplications f
...
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472650551)
Yep! There are two ways to use this data:
1. Immediately, as `(b_scan * input_hash) * sum_input_pubkeys`. This is one scalar multiplication and one EC multiplication.
2. Store it for later (either for wallet re-scans or to serve to light clients), as `input_hash * sum_input_pubkeys`. This is one EC multiplication and allows us to store all of the input data needed as 33 bytes.
If the function instead returned `input_hash * sum_input_pubkeys`, we would end up doing two EC multiplications f
...
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1472680361)
I think indeed we cannot hit this, but it's because we call `ApplyV3Rules` in a package.
When `package` is a parent+child, `has_mempool_sibling` doesn't matter since having any mempool ancestor should cause this to fail immediately. In that way, this function kind of assumes that `package` is a connected component.
If `package` has unrelated transactions, we'd let a mempool sibling slip through in this function. However, since we are calling `ApplyV3Rules` within `PreChecks`, the mempool s
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1472680361)
I think indeed we cannot hit this, but it's because we call `ApplyV3Rules` in a package.
When `package` is a parent+child, `has_mempool_sibling` doesn't matter since having any mempool ancestor should cause this to fail immediately. In that way, this function kind of assumes that `package` is a connected component.
If `package` has unrelated transactions, we'd let a mempool sibling slip through in this function. However, since we are calling `ApplyV3Rules` within `PreChecks`, the mempool s
...
📝 maflcko opened a pull request: "test: Assumeutxo with more than just coinbase transactions"
(https://github.com/bitcoin/bitcoin/pull/29354)
Currently the AU tests only check that loading a txout set with only coinbase outputs works.
Fix that by adding other transactions.
(https://github.com/bitcoin/bitcoin/pull/29354)
Currently the AU tests only check that loading a txout set with only coinbase outputs works.
Fix that by adding other transactions.
⚠️ maflcko reopened an issue: "assumeutxo: nTx and nChainTx violations in CheckBlockIndex"
(https://github.com/bitcoin/bitcoin/issues/29261)
When disabling the "test-only" assumptions in CheckBlockIndex, the check fails. This is problematic, because test-only code should not affect the behavior of the program in production.
Current diff:
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 8c583c586c..00d7ee3736 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4866,9 +4866,9 @@ void ChainstateManager::CheckBlockIndex()
unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx
...
(https://github.com/bitcoin/bitcoin/issues/29261)
When disabling the "test-only" assumptions in CheckBlockIndex, the check fails. This is problematic, because test-only code should not affect the behavior of the program in production.
Current diff:
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 8c583c586c..00d7ee3736 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4866,9 +4866,9 @@ void ChainstateManager::CheckBlockIndex()
unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx
...
💬 maflcko commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1918947945)
> test
Good idea.
On top of https://github.com/bitcoin/bitcoin/pull/29354 , the following diff should crash the node:
```diff
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 528680f2ca..e9d74ea132 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -165,6 +165,14 @@ class AssumeutxoTest(BitcoinTestFramework):
self.mini_wallet.send_self_transfer(from_node=n0)
...
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1918947945)
> test
Good idea.
On top of https://github.com/bitcoin/bitcoin/pull/29354 , the following diff should crash the node:
```diff
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 528680f2ca..e9d74ea132 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -165,6 +165,14 @@ class AssumeutxoTest(BitcoinTestFramework):
self.mini_wallet.send_self_transfer(from_node=n0)
...
🤔 theStack reviewed a pull request: "test: fix intermittent failure in p2p_v2_earlykeyresponse"
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1853577256)
Concept ACK
I think you need to also initialize the newly introduced variable in the `PeerEarlyKey` constructor, otherwise the following can happen in the `wait_until` loop:
```
AttributeError: 'PeerEarlyKey' object has no attribute 'connection_made2'
```
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1853577256)
Concept ACK
I think you need to also initialize the newly introduced variable in the `PeerEarlyKey` constructor, otherwise the following can happen in the `wait_until` loop:
```
AttributeError: 'PeerEarlyKey' object has no attribute 'connection_made2'
```
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472767521)
Added
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472767521)
Added
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472768478)
Yes, fixed here and other place I did the same.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472768478)
Yes, fixed here and other place I did the same.
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472775391)
> Maybe add tests where you (successfully) configure the node's wallet using -maxfeerate?
I dont understand what you mean by successfully configure nodes wallet.
The test you are commenting on configure node[0] with the `-maxfeerate` option.
Could you clarify your comment, thanks.
---
> Similarly, it would be good to test that both maxfee and maxfeerate requirements must be met.
Their is a test for `maxtxfee` already, and I added one for `maxfeerate`.
https://github.com/bitcoin/b
...
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472775391)
> Maybe add tests where you (successfully) configure the node's wallet using -maxfeerate?
I dont understand what you mean by successfully configure nodes wallet.
The test you are commenting on configure node[0] with the `-maxfeerate` option.
Could you clarify your comment, thanks.
---
> Similarly, it would be good to test that both maxfee and maxfeerate requirements must be met.
Their is a test for `maxtxfee` already, and I added one for `maxfeerate`.
https://github.com/bitcoin/b
...
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472778222)
Output calculation doesn't depend on the position, but using output position makes the implementation much simpler. I did try something similar to what you suggested but it ended up being really complicated to make sure I was matching up all the right amounts and was always _implicitly_ relying on output position. @achow101 pointed out it would be better and simpler to just use the output index explicitly.
This also doesn't create any dependence on output position. The user can still order th
...
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472778222)
Output calculation doesn't depend on the position, but using output position makes the implementation much simpler. I did try something similar to what you suggested but it ended up being really complicated to make sure I was matching up all the right amounts and was always _implicitly_ relying on output position. @achow101 pointed out it would be better and simpler to just use the output index explicitly.
This also doesn't create any dependence on output position. The user can still order th
...
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472785028)
`relayMinFee` is relative to the fee rate of the transaction. I believe I did the right thing by removing it and enforcing this check on `-maxfeerate` instead.
I don't think we should use the base fee value as a quantifier for the fee rate, as transaction sizes differ.
This is indicated in the issue description #29220.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472785028)
`relayMinFee` is relative to the fee rate of the transaction. I believe I did the right thing by removing it and enforcing this check on `-maxfeerate` instead.
I don't think we should use the base fee value as a quantifier for the fee rate, as transaction sizes differ.
This is indicated in the issue description #29220.
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1919057289)
> Started reviewing. Basic question first. Which RPCs are supposed to work at this stage? I was under impression, that none RPCs should understand SP addresses yet and the support come with send and receiving PRs.
Thanks for the review @S3RK ! The goal isn't that none of the RPCs should understand SP addresses, rather the goal is that _full_ RPC support is deferred to the send and receiving PRs.
> But since you've registered SP destination in `CTXDestination` variant, the SP addresses wou
...
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1919057289)
> Started reviewing. Basic question first. Which RPCs are supposed to work at this stage? I was under impression, that none RPCs should understand SP addresses yet and the support come with send and receiving PRs.
Thanks for the review @S3RK ! The goal isn't that none of the RPCs should understand SP addresses, rather the goal is that _full_ RPC support is deferred to the send and receiving PRs.
> But since you've registered SP destination in `CTXDestination` variant, the SP addresses wou
...
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472790897)
Thanks for highlighting this issue @josibake , I agree with your `BroadcastTransaction` should enforce check on both.
This will make `sendrawtransaction` returns a specific error message.
---
For this reason
`MAX_FEE_RATE_EXCEEDED` error string is now `Fee exceeds maximum configured by user.`
because if I add the option `maxtxfee` it will be misleading for in `sendrawtransaction` output
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472790897)
Thanks for highlighting this issue @josibake , I agree with your `BroadcastTransaction` should enforce check on both.
This will make `sendrawtransaction` returns a specific error message.
---
For this reason
`MAX_FEE_RATE_EXCEEDED` error string is now `Fee exceeds maximum configured by user.`
because if I add the option `maxtxfee` it will be misleading for in `sendrawtransaction` output
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472810952)
I think the check needs to be enforced on both, since we are letting a user set one or the other (or both). Removing the check entirely for one of the options feels like a regression.
It's a good point that using the base fee value as a quantifier is not ideal, but previously this code was checking that the user did not set a `maxtxfee` that was so low that the minrelay fee could never be met. Now you've removed that check, which means I can set a very low `maxtxfee` and not get a warning.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472810952)
I think the check needs to be enforced on both, since we are letting a user set one or the other (or both). Removing the check entirely for one of the options feels like a regression.
It's a good point that using the base fee value as a quantifier is not ideal, but previously this code was checking that the user did not set a `maxtxfee` that was so low that the minrelay fee could never be met. Now you've removed that check, which means I can set a very low `maxtxfee` and not get a warning.
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472815229)
Better to keep the option name in the error message:
```suggestion
return Untranslated("Fee exceeds maximum configured by user (maxtxfee)");
```
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472815229)
Better to keep the option name in the error message:
```suggestion
return Untranslated("Fee exceeds maximum configured by user (maxtxfee)");
```
📝 maflcko opened a pull request: "doc: Assert that assumed-valid blocks are not fully valid in CheckBlockIndex()"
(https://github.com/bitcoin/bitcoin/pull/29355)
It does not make sense for a block to be fully-valid and assumed-valid at the same time.
Check it in `CheckBlockIndex()` and fix the tests that violate this assumption.
(https://github.com/bitcoin/bitcoin/pull/29355)
It does not make sense for a block to be fully-valid and assumed-valid at the same time.
Check it in `CheckBlockIndex()` and fix the tests that violate this assumption.