💬 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.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1473374818)
No, SHIFT is _deselect + explore_ and CUT is _deselect + deselect + explore_, so CUT is _deselect + SHIFT_.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1473374818)
No, SHIFT is _deselect + explore_ and CUT is _deselect + deselect + explore_, so CUT is _deselect + SHIFT_.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1473380373)
Okay cool, sound great. Will adopt with the next push.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1473380373)
Okay cool, sound great. Will adopt with the next push.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1473377599)
Okay, will fix when I push next time.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1473377599)
Okay, will fix when I push next time.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1473376740)
Yeah, looks correct. I will move `result.SetSelectionsEvaluated(curr_try)` out of the loop next time I’ll push. I know there are some other reviews in flight right now.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1473376740)
Yeah, looks correct. I will move `result.SetSelectionsEvaluated(curr_try)` out of the loop next time I’ll push. I know there are some other reviews in flight right now.
💬 achow101 commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1919841411)
ACK b298242c8d495c36072415e1b95eaa7bf485a38a
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1919841411)
ACK b298242c8d495c36072415e1b95eaa7bf485a38a
🚀 achow101 merged a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(https://github.com/bitcoin/bitcoin/pull/29253)
(https://github.com/bitcoin/bitcoin/pull/29253)
💬 achow101 commented on pull request "net: enable v2transport by default":
(https://github.com/bitcoin/bitcoin/pull/29347#issuecomment-1919891645)
ACK 0bef1042ce6c459acb1de965cbccd98867a417f1
(https://github.com/bitcoin/bitcoin/pull/29347#issuecomment-1919891645)
ACK 0bef1042ce6c459acb1de965cbccd98867a417f1
📝 mzumsande converted_to_draft 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
...
🚀 achow101 merged a pull request: "net: enable v2transport by default"
(https://github.com/bitcoin/bitcoin/pull/29347)
(https://github.com/bitcoin/bitcoin/pull/29347)
👍 ryanofsky approved a pull request: "wallet: Fix migration of blank wallets"
(https://github.com/bitcoin/bitcoin/pull/28976#pullrequestreview-1854779593)
Code review ACK c11c404281d2d0e22967e30e16c0733db84f4eee
(https://github.com/bitcoin/bitcoin/pull/28976#pullrequestreview-1854779593)
Code review ACK c11c404281d2d0e22967e30e16c0733db84f4eee
💬 ryanofsky commented on pull request "wallet: Fix migration of blank wallets":
(https://github.com/bitcoin/bitcoin/pull/28976#discussion_r1473429116)
In commit "wallet: Skip key and script migration for blank wallets" (8c127ff1edb6b9a607bf1ad247893347252a38e3)
> I'm pretty sure that it was unset whenever something was imported.
This should be true but it would be good to raise an error if the blank flag is set and these records do exist. Seems like it would easy to check for, given the HasLegacyRecords() function. It would document the assumption being made by this code, and provide better error checking.
(https://github.com/bitcoin/bitcoin/pull/28976#discussion_r1473429116)
In commit "wallet: Skip key and script migration for blank wallets" (8c127ff1edb6b9a607bf1ad247893347252a38e3)
> I'm pretty sure that it was unset whenever something was imported.
This should be true but it would be good to raise an error if the blank flag is set and these records do exist. Seems like it would easy to check for, given the HasLegacyRecords() function. It would document the assumption being made by this code, and provide better error checking.