💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1473087294)
In commit "sqlite: add ability to interrupt statements" (dca874e838c2599bd24433675b291168f8e7b055)
It's a little better to pass plain `unique_ptr` instead of `unique_ptr&&`, because former guarantees unique_ptr argument will be moved-from and null afterward, while latter means the move may or may not happen depending on internals of the function. In general you want explicit && references for template programming and perfect forwarding, but should use values and & references otherwise.. Can s
...
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1473087294)
In commit "sqlite: add ability to interrupt statements" (dca874e838c2599bd24433675b291168f8e7b055)
It's a little better to pass plain `unique_ptr` instead of `unique_ptr&&`, because former guarantees unique_ptr argument will be moved-from and null afterward, while latter means the move may or may not happen depending on internals of the function. In general you want explicit && references for template programming and perfect forwarding, but should use values and & references otherwise.. Can s
...
💬 epiccurious commented on pull request "log: Don't use scientific notation in "Dumped mempool" message":
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1919542546)
@kristapsk if you prefer, I'd be happy to see it through.
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1919542546)
@kristapsk if you prefer, I'd be happy to see it through.
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1473174956)
> I'm not sure if this should be optional. Was thinking before about asserting here. Wdyt?
I don't see a benefit to aborting when the wait_callback option is true and there is no callback. There is no callback if the transaction is already in the mempool, and we are ok with ignoring the wait_callback option in that case. Would suggest changing line 95 to `if (wait_callback && node.validation_signals)`
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1473174956)
> I'm not sure if this should be optional. Was thinking before about asserting here. Wdyt?
I don't see a benefit to aborting when the wait_callback option is true and there is no callback. There is no callback if the transaction is already in the mempool, and we are ok with ignoring the wait_callback option in that case. Would suggest changing line 95 to `if (wait_callback && node.validation_signals)`
💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919554426)
> > We should not treat them in a discriminatory way.
>
> I don't see how fixing the tests in the way I've suggested would be discriminatory. It would make them work on Windows while retaining the current test for all other platforms.
Thank you! Your suggestion has been implemented.
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919554426)
> > We should not treat them in a discriminatory way.
>
> I don't see how fixing the tests in the way I've suggested would be discriminatory. It would make them work on Windows while retaining the current test for all other platforms.
Thank you! Your suggestion has been implemented.
💬 stratospher commented on pull request "test: fix intermittent failure in p2p_v2_earlykeyresponse":
(https://github.com/bitcoin/bitcoin/pull/29352#issuecomment-1919569103)
tested ACK 9642aef.
> Sounds reasonable to me, but I'd like to refer that to @stratospher, who I believe has plans to extend / rewrite the test anyway (see the WIP branch linked https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1917230742), and just fix the CI here straightforwardly.
yes, will look into it! extending this test for more v2 disconnection scenarios in [this WIP branch](https://github.com/stratospher/bitcoin/blob/6bc26fb6e1803481e773f647ddbe299fd2977877/test/functiona
...
(https://github.com/bitcoin/bitcoin/pull/29352#issuecomment-1919569103)
tested ACK 9642aef.
> Sounds reasonable to me, but I'd like to refer that to @stratospher, who I believe has plans to extend / rewrite the test anyway (see the WIP branch linked https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1917230742), and just fix the CI here straightforwardly.
yes, will look into it! extending this test for more v2 disconnection scenarios in [this WIP branch](https://github.com/stratospher/bitcoin/blob/6bc26fb6e1803481e773f647ddbe299fd2977877/test/functiona
...
💬 maflcko commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1919572281)
Rebase for fresh CI?
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1919572281)
Rebase for fresh CI?
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1919591341)
Thank you for the review @furszy!
Updated 536429372dfd9759dc8f089962f3a15a94b54751 -> cb47c334e0b3d654b5b56ae762aa2a2c33ae2743 ([noGlobalSignals_11](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_11) -> [noGlobalSignals_12](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_12), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_11..noGlobalSignals_12))
* Addressed @furszy's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1
...
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1919591341)
Thank you for the review @furszy!
Updated 536429372dfd9759dc8f089962f3a15a94b54751 -> cb47c334e0b3d654b5b56ae762aa2a2c33ae2743 ([noGlobalSignals_11](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_11) -> [noGlobalSignals_12](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_12), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_11..noGlobalSignals_12))
* Addressed @furszy's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1
...
💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1919611531)
Concept ACK
My scenario for using both `maxtxfee` and `maxfeerate` would be slightly different. For example looking at the mempool in January, I might generally be okay with paying up to 50 sat/vB, but not a higher feerate, because I expect that it will generally suffice to get a confirmation within 24h, but at such a high feerate, I would not want to make a transaction with more than 10 inputs. So, I’d delimit the feerate itself on the one hand, and then also limit the `maxtxfee` to 35'000 s
...
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1919611531)
Concept ACK
My scenario for using both `maxtxfee` and `maxfeerate` would be slightly different. For example looking at the mempool in January, I might generally be okay with paying up to 50 sat/vB, but not a higher feerate, because I expect that it will generally suffice to get a confirmation within 24h, but at such a high feerate, I would not want to make a transaction with more than 10 inputs. So, I’d delimit the feerate itself on the one hand, and then also limit the `maxtxfee` to 35'000 s
...
💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919617791)
> > Can you add exact steps to reproduce?
>
> ```
> $ cat /etc/os-release | head -1
> PRETTY_NAME="Ubuntu 22.04.3 LTS"
> $ x86_64-w64-mingw32-g++-posix --version
> x86_64-w64-mingw32-g++-posix (GCC) 10-posix 20220113
> Copyright (C) 2020 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
> $ make -C depends -j $(nproc) HOST=x86_64-w64-mingw32 NO_QT
...
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919617791)
> > Can you add exact steps to reproduce?
>
> ```
> $ cat /etc/os-release | head -1
> PRETTY_NAME="Ubuntu 22.04.3 LTS"
> $ x86_64-w64-mingw32-g++-posix --version
> x86_64-w64-mingw32-g++-posix (GCC) 10-posix 20220113
> Copyright (C) 2020 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
> $ make -C depends -j $(nproc) HOST=x86_64-w64-mingw32 NO_QT
...
💬 theStack commented on pull request "test: make v2transport arg in addconnection mandatory and few cleanups":
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1919629659)
Concept ACK
Another possible alternative for `addconnection`'s third parameter handling would be to take as default whatever is set by `-v2transport`, like we already do it for `addnode` (i.e. `bool use_v2transport = self.MaybeArg<bool>(2).value_or(node_v2transport);`).
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1919629659)
Concept ACK
Another possible alternative for `addconnection`'s third parameter handling would be to take as default whatever is set by `-v2transport`, like we already do it for `addnode` (i.e. `bool use_v2transport = self.MaybeArg<bool>(2).value_or(node_v2transport);`).
💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919640104)
> I also tried this locally, and the file is not a nullptr.
I agree. That is how it works on Linux in the Wine environment.
On Windows, being linked to `msvcrt.dll`, it fails.
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919640104)
> I also tried this locally, and the file is not a nullptr.
I agree. That is how it works on Linux in the Wine environment.
On Windows, being linked to `msvcrt.dll`, it fails.
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473270005)
Summarizing my thoughts from some offline discussion with @ismaelsadeeq :
* I'm fairly certain we don't check `relayMinFee` when creating a transaction, but it would be good to confirm this (even better, to have a functional test!)
* If the user can set both `relayMinFee` and `maxtxfee`, I think we need to have a sanity check at startup to make sure they are not setting insane values that could result in them creating stuck transactions
* I think its okay to use a "fake transaction size" fo
...
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473270005)
Summarizing my thoughts from some offline discussion with @ismaelsadeeq :
* I'm fairly certain we don't check `relayMinFee` when creating a transaction, but it would be good to confirm this (even better, to have a functional test!)
* If the user can set both `relayMinFee` and `maxtxfee`, I think we need to have a sanity check at startup to make sure they are not setting insane values that could result in them creating stuck transactions
* I think its okay to use a "fake transaction size" fo
...
💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919669199)
> > I also tried this locally, and the file is not a nullptr.
>
> I agree. That is how it works on Linux in the Wine environment.
>
> On Windows, being linked to `msvcrt.dll`, it fails.
Yes, either the file is a nullptr, in which case the test already fails, or it is not, in which case the test passes. In any case, the added assert in the dropped commit isn't needed here or elsewhere, and doesn't change the outcome here.
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919669199)
> > I also tried this locally, and the file is not a nullptr.
>
> I agree. That is how it works on Linux in the Wine environment.
>
> On Windows, being linked to `msvcrt.dll`, it fails.
Yes, either the file is a nullptr, in which case the test already fails, or it is not, in which case the test passes. In any case, the added assert in the dropped commit isn't needed here or elsewhere, and doesn't change the outcome here.
🤔 murchandamus reviewed a pull request: "Wallet: Add `maxfeerate` wallet startup option"
(https://github.com/bitcoin/bitcoin/pull/29278#pullrequestreview-1854463255)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29278#pullrequestreview-1854463255)
Concept ACK
💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473230950)
We are comparing two feerates, should this perhaps be:
```suggestion
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("fee rate cannot be more than wallet max tx fee rate (%s)", pwallet->m_max_tx_fee_rate.ToString()));
```
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473230950)
We are comparing two feerates, should this perhaps be:
```suggestion
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("fee rate cannot be more than wallet max tx fee rate (%s)", pwallet->m_max_tx_fee_rate.ToString()));
```
💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473259190)
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
I am a bit confused. You marked this as resolved and said you added this, but I don’t see it in the current version. Did you fix it in your working branch but haven’t pushed yet?
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473259190)
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
I am a bit confused. You marked this as resolved and said you added this, but I don’t see it in the current version. Did you fix it in your working branch but haven’t pushed yet?
💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473237266)
In 7664ec9bd744ab5ae403a87cd9d2aa3de747d278 (wallet: add `maxfeerate` wallet startup option):
`walletInstance->m_default_max_tx_fee` indicates to me that the value is the _default_ if the user does not customize the value, so it is confusing to me that the user value is assigned to a variable with "default" in its name.
```suggestion
walletInstance->m_max_tx_fee = max_fee.value();
```
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473237266)
In 7664ec9bd744ab5ae403a87cd9d2aa3de747d278 (wallet: add `maxfeerate` wallet startup option):
`walletInstance->m_default_max_tx_fee` indicates to me that the value is the _default_ if the user does not customize the value, so it is confusing to me that the user value is assigned to a variable with "default" in its name.
```suggestion
walletInstance->m_max_tx_fee = max_fee.value();
```
💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473247758)
In 7664ec9bd744ab5ae403a87cd9d2aa3de747d278 (wallet: add `maxfeerate` wallet startup option):
If I did not miscalculate this feerate translates to 10'000 sat/vB. A single input two output P2TR transaction would incur a cost of over 15 m₿ at that feerate. I could see how this might be reached if we CPFP a big transaction with a small transaction, but for a standalone transaction this limit fees too high for a failsafe.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473247758)
In 7664ec9bd744ab5ae403a87cd9d2aa3de747d278 (wallet: add `maxfeerate` wallet startup option):
If I did not miscalculate this feerate translates to 10'000 sat/vB. A single input two output P2TR transaction would incur a cost of over 15 m₿ at that feerate. I could see how this might be reached if we CPFP a big transaction with a small transaction, but for a standalone transaction this limit fees too high for a failsafe.
💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473269857)
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
If you first build a transaction that succeeds and then try to build a transaction that fails, this could also be caused by the success of the first transaction. Perhaps it would be better to turn around the order:
1. Try to send at a feerate _above_ the `maxfeerate`, since that fails the wallet is still in the same state.
2. Try to send at a feerate _just below_ the `maxfeerate` and sh
...
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473269857)
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
If you first build a transaction that succeeds and then try to build a transaction that fails, this could also be caused by the success of the first transaction. Perhaps it would be better to turn around the order:
1. Try to send at a feerate _above_ the `maxfeerate`, since that fails the wallet is still in the same state.
2. Try to send at a feerate _just below_ the `maxfeerate` and sh
...
💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473276807)
In d4dceeafc2292cd050cb10e308d73bc8ef84201a (wallet: test: add new transaction error type):
This seems strictly less informative. I would second @josibake in the suggestion to keep mentioning the `maxtxfee` here. Maybe I’m missing something, could you elaborate why you would push that back to a follow-up?
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473276807)
In d4dceeafc2292cd050cb10e308d73bc8ef84201a (wallet: test: add new transaction error type):
This seems strictly less informative. I would second @josibake in the suggestion to keep mentioning the `maxtxfee` here. Maybe I’m missing something, could you elaborate why you would push that back to a follow-up?