💬 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.
📝 mzumsande opened a pull request: "test: use v2 everywhere for P2PConnection if --v2transport is enabled"
(https://github.com/bitcoin/bitcoin/pull/29358)
#24748 added v2 transport to the python `P2PConnection`, but so far each test that wants to make use of it needs to enable it on an individual basis.
This PR changes it so that if the test suite is run with `--v2transport` option, v2 is used in each test by default, not only for connections between two bitcoind instances as before, but also wherever `P2PConnection` is used. Individual tests can override this global option.
To do that, a few tests need to be adjusted, which is done in the fir
...
(https://github.com/bitcoin/bitcoin/pull/29358)
#24748 added v2 transport to the python `P2PConnection`, but so far each test that wants to make use of it needs to enable it on an individual basis.
This PR changes it so that if the test suite is run with `--v2transport` option, v2 is used in each test by default, not only for connections between two bitcoind instances as before, but also wherever `P2PConnection` is used. Individual tests can override this global option.
To do that, a few tests need to be adjusted, which is done in the fir
...
💬 josibake commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1919780358)
I was tinkering around with this and had a few thoughts:
* Being able to specify a `start_height` sounds like something that would be generally useful for all indexes, not just this one. I've been toying with the idea of what it would take to have a "pruned" txindex and I would need to be able to start from a specific height for that. @fjahr I took the branch you started and got it working with a little tinkering here: https://github.com/josibake/bitcoin/tree/add-start-height-to-base-index .
...
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1919780358)
I was tinkering around with this and had a few thoughts:
* Being able to specify a `start_height` sounds like something that would be generally useful for all indexes, not just this one. I've been toying with the idea of what it would take to have a "pruned" txindex and I would need to be able to start from a specific height for that. @fjahr I took the branch you started and got it working with a little tinkering here: https://github.com/josibake/bitcoin/tree/add-start-height-to-base-index .
...
💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1919780817)
> This PR feels like it's the wrong approach to dealing with this particular database failure. It shouldn't be possible for a transaction commit or abort to currently fail in way that isn't catastrophic. In that case, we should be shutting the wallet rather than reopening the database connection and pretending that nothing happened. This could potentially lead to a state where the wallet's in-memory state does not match the database, which is really bad and could lead to loss of funds.
It see
...
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1919780817)
> This PR feels like it's the wrong approach to dealing with this particular database failure. It shouldn't be possible for a transaction commit or abort to currently fail in way that isn't catastrophic. In that case, we should be shutting the wallet rather than reopening the database connection and pretending that nothing happened. This could potentially lead to a state where the wallet's in-memory state does not match the database, which is really bad and could lead to loss of funds.
It see
...
🤔 mzumsande reviewed a pull request: "test: Assumeutxo with more than just coinbase transactions"
(https://github.com/bitcoin/bitcoin/pull/29354#pullrequestreview-1854661101)
Concept ACK, I did something very similar locally when investigating #29261.
(https://github.com/bitcoin/bitcoin/pull/29354#pullrequestreview-1854661101)
Concept ACK, I did something very similar locally when investigating #29261.
👍 kristapsk approved a pull request: "net: enable v2transport by default"
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1854666915)
utACK 0bef1042ce6c459acb1de965cbccd98867a417f1
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1854666915)
utACK 0bef1042ce6c459acb1de965cbccd98867a417f1
💬 kristapsk commented on pull request "test: make v2transport arg in addconnection mandatory and few cleanups":
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1919804582)
> * make `v2transport` argument in `addconnection` RPC mandatory.
This will break compatibility for third party software / scripts written against previous behaviour where there was no such mandatory argument. What are benefits?
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1919804582)
> * make `v2transport` argument in `addconnection` RPC mandatory.
This will break compatibility for third party software / scripts written against previous behaviour where there was no such mandatory argument. What are benefits?
💬 mzumsande commented on pull request "test: make v2transport arg in addconnection mandatory and few cleanups":
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1919812989)
> This will break compatibility for third party software / scripts written against previous behaviour where there was no such mandatory argument. What are benefits?
`addconnection` is a debug-only command that is only used by our functional test framework (to make "fake" automatic outbound connections). It has never been announced to the public, is only enabled for regtest and I don't see why any third party would ever need to use it. Did you maybe confuse it with the widely used `addnode` rp
...
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1919812989)
> This will break compatibility for third party software / scripts written against previous behaviour where there was no such mandatory argument. What are benefits?
`addconnection` is a debug-only command that is only used by our functional test framework (to make "fake" automatic outbound connections). It has never been announced to the public, is only enabled for regtest and I don't see why any third party would ever need to use it. Did you maybe confuse it with the widely used `addnode` rp
...
💬 kristapsk commented on pull request "test: make v2transport arg in addconnection mandatory and few cleanups":
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1919821003)
Ok, didn't notice it is regtest only. Probably `help addconnection` should give error instead of usage information if not running against regtest.
With v26.0.0 you get this against mainnet:
```
bitcoin@odroid:~$ bitcoin-cli help | grep addconnection
bitcoin@odroid:~$ bitcoin-cli help addconnection
addconnection "address" "connection_type"
Open an outbound connection to a specified node. This RPC is for testing only.
Arguments:
1. address (string, required) The IP
...
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1919821003)
Ok, didn't notice it is regtest only. Probably `help addconnection` should give error instead of usage information if not running against regtest.
With v26.0.0 you get this against mainnet:
```
bitcoin@odroid:~$ bitcoin-cli help | grep addconnection
bitcoin@odroid:~$ bitcoin-cli help addconnection
addconnection "address" "connection_type"
Open an outbound connection to a specified node. This RPC is for testing only.
Arguments:
1. address (string, required) The IP
...
🤔 murchandamus reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1854679982)
Thanks @sipa, good improvements, will amend when the other review in flight comes back.
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1854679982)
Thanks @sipa, good improvements, will amend when the other review in flight comes back.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1473374021)
While I agree that CoinGrinder already does work that scales with the size of the UTXO pool, I would argue that CoinGrinder should never need more than 100'000 tries to find _a_ solution. We can only use up to ~1740 inputs in a standard transaction, so if a wallet can’t scrounge up enough funds after traversing 100'000 candidate input sets, I’d say it has other issues.
Happy to change both of these in a follow-up, but wouldn’t consider it necessary for this PR to land.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1473374021)
While I agree that CoinGrinder already does work that scales with the size of the UTXO pool, I would argue that CoinGrinder should never need more than 100'000 tries to find _a_ solution. We can only use up to ~1740 inputs in a standard transaction, so if a wallet can’t scrounge up enough funds after traversing 100'000 candidate input sets, I’d say it has other issues.
Happy to change both of these in a follow-up, but wouldn’t consider it necessary for this PR to land.