💬 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?
💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473256087)
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
Given that we might use a small transaction to bump a big transaction, we might actually want to have a very large feerate on the child to achieve a reasonable mining score for the parent-child package. Would it perhaps make sense to generally lower the default maxfeerate but then compare it with the resulting mining score instead of the new transaction’s individual feerate?
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473256087)
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
Given that we might use a small transaction to bump a big transaction, we might actually want to have a very large feerate on the child to achieve a reasonable mining score for the parent-child package. Would it perhaps make sense to generally lower the default maxfeerate but then compare it with the resulting mining score instead of the new transaction’s individual feerate?
💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473271780)
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
One transaction but closer to the limit should suffice:
```suggestion
self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), amount=1, fee_rate=9)
```
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473271780)
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):
One transaction but closer to the limit should suffice:
```suggestion
self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), amount=1, fee_rate=9)
```
💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473275731)
In d4dceeafc2292cd050cb10e308d73bc8ef84201a (wallet: test: add new transaction error type):
It isn’t clear to me why we would first ship a less descriptive new error message in this PR and then immediately fix it in a follow-up. It seems better to ship a descriptive error message right here.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473275731)
In d4dceeafc2292cd050cb10e308d73bc8ef84201a (wallet: test: add new transaction error type):
It isn’t clear to me why we would first ship a less descriptive new error message in this PR and then immediately fix it in a follow-up. It seems better to ship a descriptive error message right here.
💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473280414)
I realized now that you are adding the new Error type in the third PR. It would make more sense to me if the new Error type were introduced first, and then the test that uses it would get added rather than adding the test and then later introducing the new Error type.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473280414)
I realized now that you are adding the new Error type in the third PR. It would make more sense to me if the new Error type were introduced first, and then the test that uses it would get added rather than adding the test and then later introducing the new Error type.
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473283667)
> Perhaps just retain a version of the old check by enforcing that maxtxfee is set to at least CAmount FLOOR_MAXTXFEE{1000} (a conversion from the default min relay feerate)?
This doesn't cover the case where a user sets a `maxtxfee` and a custom `relayMinFee`. Admittedly, the old check wasn't perfect in that the user could still create very large transactions that *might* still cause a problem. I think the best would be have a sanity check at startup (either the old one, or a new one with a
...
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473283667)
> Perhaps just retain a version of the old check by enforcing that maxtxfee is set to at least CAmount FLOOR_MAXTXFEE{1000} (a conversion from the default min relay feerate)?
This doesn't cover the case where a user sets a `maxtxfee` and a custom `relayMinFee`. Admittedly, the old check wasn't perfect in that the user could still create very large transactions that *might* still cause a problem. I think the best would be have a sanity check at startup (either the old one, or a new one with a
...
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1919698019)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1919698019)
Concept ACK
💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919713989)
> > > 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.
You are right. That commit was my mistake.
FWI
...
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919713989)
> > > 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.
You are right. That commit was my mistake.
FWI
...
💬 achow101 commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1919735812)
> It wouldn't be ideal if the error is still recoverable (e.g. [SQLITE_BUSY](https://sqlite.org/rescode.html#busy) if we ever introduce concurrent txns).
> At least in this scenario, where we are destroying the object that created the transaction, I believe triggering the database connection reset is acceptable as a first measure.
If we do have recoverable errors, then we should be handling them, not hitting them with the hammer of closing and reopening the database.
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1919735812)
> It wouldn't be ideal if the error is still recoverable (e.g. [SQLITE_BUSY](https://sqlite.org/rescode.html#busy) if we ever introduce concurrent txns).
> At least in this scenario, where we are destroying the object that created the transaction, I believe triggering the database connection reset is acceptable as a first measure.
If we do have recoverable errors, then we should be handling them, not hitting them with the hammer of closing and reopening the database.
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1919747352)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1919747352)
Rebased.
💬 kristapsk commented on pull request "log: Don't use scientific notation in log messages":
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1919775494)
Changed also other message and added rouding to ms precision.
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1919775494)
Changed also other message and added rouding to ms precision.