💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479649957)
in "node: BroadcastTransaction should also check tx fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):
Don't we have a `DEFAULT_TRANSACTION_MAXFEERATE`? Wouldn't it be better to use that here instead of 0?
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479649957)
in "node: BroadcastTransaction should also check tx fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):
Don't we have a `DEFAULT_TRANSACTION_MAXFEERATE`? Wouldn't it be better to use that here instead of 0?
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479650353)
in "node: BroadcastTransaction should also check tx fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):
Same comment as above regarding defaults
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479650353)
in "node: BroadcastTransaction should also check tx fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):
Same comment as above regarding defaults
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479651121)
in "node: BroadcastTransaction should also check tx fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):
Same comment regarding defaults vs 0
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479651121)
in "node: BroadcastTransaction should also check tx fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):
Same comment regarding defaults vs 0
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479686164)
in "wallet: test: enforce -maxfeerate on wallet transactions" (https://github.com/bitcoin/bitcoin/commit/eadc3a4500d6b7be263a2981602c20f6e5eb13a4):
What's the reason for changing the tests instead of adding a new test case for `maxfeerate`? Seems like we should be testing both rather than changing the old `maxtxfee` tests to now check `maxtxfeerate`.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479686164)
in "wallet: test: enforce -maxfeerate on wallet transactions" (https://github.com/bitcoin/bitcoin/commit/eadc3a4500d6b7be263a2981602c20f6e5eb13a4):
What's the reason for changing the tests instead of adding a new test case for `maxfeerate`? Seems like we should be testing both rather than changing the old `maxtxfee` tests to now check `maxtxfeerate`.
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479684395)
in "wallet: test: enforce -maxfeerate on wallet transactions" (https://github.com/bitcoin/bitcoin/pull/29278/commits/eadc3a4500d6b7be263a2981602c20f6e5eb13a4):
This should be `MAX_FEE_RATE_EXCEEDED`, right?
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479684395)
in "wallet: test: enforce -maxfeerate on wallet transactions" (https://github.com/bitcoin/bitcoin/pull/29278/commits/eadc3a4500d6b7be263a2981602c20f6e5eb13a4):
This should be `MAX_FEE_RATE_EXCEEDED`, right?
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479669661)
in "wallet: add maxfeerate wallet startup option" (https://github.com/bitcoin/bitcoin/pull/29278/commits/4165dc41289fd337e279f3986dfc1a84345c900e):
Why are you removing the `-maxtxfee` test? Why not just add a new test case for `-maxfeerate`?
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479669661)
in "wallet: add maxfeerate wallet startup option" (https://github.com/bitcoin/bitcoin/pull/29278/commits/4165dc41289fd337e279f3986dfc1a84345c900e):
Why are you removing the `-maxtxfee` test? Why not just add a new test case for `-maxfeerate`?
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479691126)
in "wallet: rename m_default_max_tx_fee to m_max_tx_fee" (https://github.com/bitcoin/bitcoin/pull/29278/commits/a1497af6fc11009839ef86de1bf2da5cf99ad489):
Agree that this is way less confusing as a name, but probably still good to have a commit message explaining your reasoning. I'd also suggest moving this commit to the beginning, before any of the tests are changed and also add the refactor label to the commit message.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479691126)
in "wallet: rename m_default_max_tx_fee to m_max_tx_fee" (https://github.com/bitcoin/bitcoin/pull/29278/commits/a1497af6fc11009839ef86de1bf2da5cf99ad489):
Agree that this is way less confusing as a name, but probably still good to have a commit message explaining your reasoning. I'd also suggest moving this commit to the beginning, before any of the tests are changed and also add the refactor label to the commit message.
💬 epiccurious commented on pull request "test: speedup bip324_cipher.py unit test":
(https://github.com/bitcoin/bitcoin/pull/29390#issuecomment-1929537124)
> a cleaner alternative would probably be to introduce a special seek/skip_packets method and not touch the encrypt/decrypt routines
Why did you choose this approach rather than adding a seek/skip_packet method?
(https://github.com/bitcoin/bitcoin/pull/29390#issuecomment-1929537124)
> a cleaner alternative would probably be to introduce a special seek/skip_packets method and not touch the encrypt/decrypt routines
Why did you choose this approach rather than adding a seek/skip_packet method?
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479783373)
Yes 👍🏾 its better that way.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479783373)
Yes 👍🏾 its better that way.
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479796113)
in "wallet: add maxfeerate wallet startup option" https://github.com/bitcoin/bitcoin/commit/4165dc41289fd337e279f3986dfc1a84345c900e
After adding `maxfeerate` option we now only check if the fee rate user set is less `minrelaytxfee`, less than wallet min fee, or whether it exceed `maxfeerate`.
So their is no need to check for `maxtxfee` here, because its not enforced here.
Will add this info in commit message, thanks
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479796113)
in "wallet: add maxfeerate wallet startup option" https://github.com/bitcoin/bitcoin/commit/4165dc41289fd337e279f3986dfc1a84345c900e
After adding `maxfeerate` option we now only check if the fee rate user set is less `minrelaytxfee`, less than wallet min fee, or whether it exceed `maxfeerate`.
So their is no need to check for `maxtxfee` here, because its not enforced here.
Will add this info in commit message, thanks
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479796527)
Yes 👍🏾
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479796527)
Yes 👍🏾
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479814983)
Because after https://github.com/bitcoin/bitcoin/commit/eadc3a4500d6b7be263a2981602c20f6e5eb13a4 we now check for `maxfeerate` threshold first before checking `maxtxfee` threshold.
And the specified fee rate is above default `maxferate`, as such it will be hit first.
But I agree, I will add another test where specified bump fee rate is below the `maxfeerate` but we hit `maxtxfee` due to size or maybe just restart with `maxtxfee` set to a lower value ?
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479814983)
Because after https://github.com/bitcoin/bitcoin/commit/eadc3a4500d6b7be263a2981602c20f6e5eb13a4 we now check for `maxfeerate` threshold first before checking `maxtxfee` threshold.
And the specified fee rate is above default `maxferate`, as such it will be hit first.
But I agree, I will add another test where specified bump fee rate is below the `maxfeerate` but we hit `maxtxfee` due to size or maybe just restart with `maxtxfee` set to a lower value ?
⚠️ glozow opened an issue: "wallet_reorgrestore.py failed"
(https://github.com/bitcoin/bitcoin/issues/29392)
### Motivation
https://cirrus-ci.com/task/5616118744743936
```
test 2024-02-06T11:27:14.369000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/ci_contai
...
(https://github.com/bitcoin/bitcoin/issues/29392)
### Motivation
https://cirrus-ci.com/task/5616118744743936
```
test 2024-02-06T11:27:14.369000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/ci_contai
...
🤔 murchandamus reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1862978564)
Responded to feedback from @sipa and @sr-gi
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1862978564)
Responded to feedback from @sipa and @sr-gi
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478391942)
Rephrased to mention again all three possible state transitions with example
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478391942)
Rephrased to mention again all three possible state transitions with example
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1479832383)
Yes I think that would be equivalent.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1479832383)
Yes I think that would be equivalent.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478374197)
Added!
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478374197)
Added!
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478362138)
Thanks, I’ve improved that sentence.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478362138)
Thanks, I’ve improved that sentence.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1479813643)
@sr-gi: I did not count the root node, since we do not add a UTXO in the root node, so I did not count it among the inclusion branch nodes.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1479813643)
@sr-gi: I did not count the root node, since we do not add a UTXO in the root node, so I did not count it among the inclusion branch nodes.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478371817)
Thanks, good catch
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478371817)
Thanks, good catch