💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218179793)
Ok understood. In that case, would it make sense to be very explicit about what this connection is about? I mean user agent could essentially be "I am going to send you one TX and then disconnect". Additional behavior could even be encoded on the recipient side, maybe to ensure we don't disconnect prematurely ?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218179793)
Ok understood. In that case, would it make sense to be very explicit about what this connection is about? I mean user agent could essentially be "I am going to send you one TX and then disconnect". Additional behavior could even be encoded on the recipient side, maybe to ensure we don't disconnect prematurely ?
💬 hebasto commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1576941978)
https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214342650:
> I think the approach in the branch is better for two reasons...
I agree with both of them.
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1576941978)
https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214342650:
> I think the approach in the branch is better for two reasons...
I agree with both of them.
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1576942166)
General question about this: what will be the impact on tor/i2p nodes? are there even enough hidden services nodes with inbound slots to support this? In a world where all TXs are broadcast to the network through "exit nodes" are there any other possible drawbacks ?
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1576942166)
General question about this: what will be the impact on tor/i2p nodes? are there even enough hidden services nodes with inbound slots to support this? In a world where all TXs are broadcast to the network through "exit nodes" are there any other possible drawbacks ?
👍 Sjors approved a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1462675329)
utACK c79539fc20d902b5f9e098cc996c4e27c8e5b8c5
Everything since my last review is a rebase, except 95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94, so I only re-reviewed that, plus 70a2bb75442daaef5685728b4b03c7b2fe49fd38.
I'd still like to see a test and/or release note to cover the case where a user upgrades, then downgrades and encrypts their wallet and then upgrades. See https://github.com/bitcoin/bitcoin/pull/26728/commits/95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94#r1081289252 But it can wait
...
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1462675329)
utACK c79539fc20d902b5f9e098cc996c4e27c8e5b8c5
Everything since my last review is a rebase, except 95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94, so I only re-reviewed that, plus 70a2bb75442daaef5685728b4b03c7b2fe49fd38.
I'd still like to see a test and/or release note to cover the case where a user upgrades, then downgrades and encrypts their wallet and then upgrades. See https://github.com/bitcoin/bitcoin/pull/26728/commits/95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94#r1081289252 But it can wait
...
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218151088)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94 `// Automatically`
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218151088)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94 `// Automatically`
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218166104)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: can we emit a warning the in the `else` case?
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218166104)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: can we emit a warning the in the `else` case?
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218161082)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: why not loop over `xpubs`?
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218161082)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: why not loop over `xpubs`?
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218168069)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: it's better if the caller checks this flag. It makes it easier to implement a "try again" feature later for folks with complicated wallets where somehow the upgrade fails to find a best xpub. Can wait for followup though.
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218168069)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: it's better if the caller checks this flag. It makes it easier to implement a "try again" feature later for folks with complicated wallets where somehow the upgrade fails to find a best xpub. Can wait for followup though.
💬 furszy commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1218214833)
> What do you mean with "different chains"? `vSortedHeight` could contain blocks with the same height x (forks), but I can't see how these could have made it into the block index without having a predecessor of height x - 1 at the time they were added. Also, these shouldn't make the added check fail.
yeah ok. nvm. Got confused by the early return (I thought it in the other way around).
The check will have the previous block at the same height, so `pindex->nHeight > previous_index->nHeight +
...
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1218214833)
> What do you mean with "different chains"? `vSortedHeight` could contain blocks with the same height x (forks), but I can't see how these could have made it into the block index without having a predecessor of height x - 1 at the time they were added. Also, these shouldn't make the added check fail.
yeah ok. nvm. Got confused by the early return (I thought it in the other way around).
The check will have the previous block at the same height, so `pindex->nHeight > previous_index->nHeight +
...
💬 dangershony commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1576991661)
Having tried to setup a customs signet I must ACK the concept.
> But both those have the same problem: they're giving you behaviour that you won't see on mainnet.
You can totally get 30 sec blocks on mainnet sure its very uncommon but valid none the less.
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1576991661)
Having tried to setup a customs signet I must ACK the concept.
> But both those have the same problem: they're giving you behaviour that you won't see on mainnet.
You can totally get 30 sec blocks on mainnet sure its very uncommon but valid none the less.
💬 MatthewLM commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1577007627)
Is it a problem prefixing public keys with `0x`? I'm not overly fussed on the solution as long as the ambiguity is removed.
> It seems like this could break much of the little scripting tooling we have available for bitcoin
If these tools read these ambiguous numbers/data from ASM, then they are already broken, so this needs fixing one way or another.
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1577007627)
Is it a problem prefixing public keys with `0x`? I'm not overly fussed on the solution as long as the ambiguity is removed.
> It seems like this could break much of the little scripting tooling we have available for bitcoin
If these tools read these ambiguous numbers/data from ASM, then they are already broken, so this needs fixing one way or another.
💬 Sjors commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1218240443)
I'm a bit confused what this comparison is doing.
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1218240443)
I'm a bit confused what this comparison is doing.
💬 glozow commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1218244764)
I could add the modified fee to the RPC result? I don't feel strongly, happy to change however people want.
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1218244764)
I could add the modified fee to the RPC result? I don't feel strongly, happy to change however people want.
💬 furszy commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1218246518)
If you load a wallet from an external path (not the wallet directory), the wallet name is set to the external path.
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1218246518)
If you load a wallet from an external path (not the wallet directory), the wallet name is set to the external path.
💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1577039004)
Adding a fuzz target that compares this to the real thing seems useful. Given that this PR changes `dumpwallet` to rely on the new parser, I think we should do that sooner rather than later. Otherwise I suggest changing e88ad38ac2f3e43de54a332965b13eb7cc27b91f so that the read-only parser is optional, and then change the tests to always use it.
It seems a bit daunting to compare the low level DBD parsing code in this PR to the real thing. Having a fuzzer also reduces the need to be very thoro
...
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1577039004)
Adding a fuzz target that compares this to the real thing seems useful. Given that this PR changes `dumpwallet` to rely on the new parser, I think we should do that sooner rather than later. Otherwise I suggest changing e88ad38ac2f3e43de54a332965b13eb7cc27b91f so that the read-only parser is optional, and then change the tests to always use it.
It seems a bit daunting to compare the low level DBD parsing code in this PR to the real thing. Having a fuzzer also reduces the need to be very thoro
...
💬 Sjors commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1218262658)
So this applies to wallets outside the datadir, but not to the original default wallet sitting in the root of the data dir (instead of /wallets)?
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1218262658)
So this applies to wallets outside the datadir, but not to the original default wallet sitting in the root of the data dir (instead of /wallets)?
⚠️ miketwenty1 opened an issue: "bitcoin-cli requires unnecessary data field for certain commands"
(https://github.com/bitcoin/bitcoin/issues/27828)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When executing: `bitcoin-cli walletcreatefundedpsbt` or `bitcoin-cli createpsbt` without args you will get a help output that looks like this:
```
Creates and funds a transaction in the Partially Signed Transaction format.
Implements the Creator and Updater roles.
All existing inputs must either have their previous output transaction be in the wallet
or be in the UTXO set. Solving dat
...
(https://github.com/bitcoin/bitcoin/issues/27828)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When executing: `bitcoin-cli walletcreatefundedpsbt` or `bitcoin-cli createpsbt` without args you will get a help output that looks like this:
```
Creates and funds a transaction in the Partially Signed Transaction format.
Implements the Creator and Updater roles.
All existing inputs must either have their previous output transaction be in the wallet
or be in the UTXO set. Solving dat
...
💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218267977)
> Yes, if the peer sends us unsolicited PONG without us sending PING before that.
I don't think the peer can guess the right ping nonce reliably , can it?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218267977)
> Yes, if the peer sends us unsolicited PONG without us sending PING before that.
I don't think the peer can guess the right ping nonce reliably , can it?
💬 MarcoFalke commented on issue "bitcoin-cli requires unnecessary data field for certain commands":
(https://github.com/bitcoin/bitcoin/issues/27828#issuecomment-1577052721)
Should be a trivial patch to change the bool from true to false, no? Mind creating a pull?
(https://github.com/bitcoin/bitcoin/issues/27828#issuecomment-1577052721)
Should be a trivial patch to change the bool from true to false, no? Mind creating a pull?
💬 furszy commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1218275412)
Check https://github.com/bitcoin/bitcoin/pull/26740#pullrequestreview-1252729613.
IIRC (because passed some time since I tested this), you can have several wallets inside the datadir root, and trying to migrate them cause the error if the old `wallet.dat` is there, because the process is not using the wallet name, it's using the db path and appending the "wallet.dat" name at the end.
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1218275412)
Check https://github.com/bitcoin/bitcoin/pull/26740#pullrequestreview-1252729613.
IIRC (because passed some time since I tested this), you can have several wallets inside the datadir root, and trying to migrate them cause the error if the old `wallet.dat` is there, because the process is not using the wallet name, it's using the db path and appending the "wallet.dat" name at the end.