👍 kristapsk approved a pull request: "net: enable v2transport by default"
(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?

💬 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
...

💬 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
...

🤔 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.

💬 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.

💬 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_.

💬 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.

💬 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.

💬 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.

💬 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

🚀 achow101 merged a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(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

📝 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
...

🚀 achow101 merged a pull request: "net: enable v2transport by default"
(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

💬 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.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1919923411)
Rebased cb47c334e0b3d654b5b56ae762aa2a2c33ae2743 -> 6752d218caeed1111f2520130c156b9ef42ba805 ([noGlobalSignals_12](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_12) -> [noGlobalSignals_13](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_13), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_12..noGlobalSignals_13))

* Fixed silent merge conflict with https://github.com/bitcoin/bitcoin/pull/28170

💬 achow101 commented on pull request "Nuke adjusted time from validation (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1919931046)
ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2

🚀 achow101 merged a pull request: "Nuke adjusted time from validation (attempt 2)"
(https://github.com/bitcoin/bitcoin/pull/28956)

🤔 TheCharlatan reviewed a pull request: "Nuke adjusted time from validation (attempt 2)"
(https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1854827936)
ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2