💬 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.
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1577096803)
@instagibbs added some more info to the OP, but feel free to reach on `#bitcoin-core-dev` to clarify things. Once a local transaction is is received, 5 such connections will be made right away. If we still consider the transaction unbroadcast after 10-15 mins, then it will be retried (5 new connections). These 10-15 mins are how `master` does things now but I think if sensitive relay is used this can be made more aggressive and/or increase the number of peers we broadcast to. E.g. after 3 minute
...
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1577096803)
@instagibbs added some more info to the OP, but feel free to reach on `#bitcoin-core-dev` to clarify things. Once a local transaction is is received, 5 such connections will be made right away. If we still consider the transaction unbroadcast after 10-15 mins, then it will be retried (5 new connections). These 10-15 mins are how `master` does things now but I think if sensitive relay is used this can be made more aggressive and/or increase the number of peers we broadcast to. E.g. after 3 minute
...
📝 miketwenty1 opened a pull request: "rpc: fix data optionality for RPC calls."
(https://github.com/bitcoin/bitcoin/pull/27829)
The "data" field without outputs was marked as "required" in the help docs when using bitcoin-cli. This field when left off worked as an intended optional OP_RETURN. closes #27828.
Motivation: It is hard to understand that "data" is actually optional for commands like `createpsbt` and `walletcreatefundedpsbt`.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull reques
...
(https://github.com/bitcoin/bitcoin/pull/27829)
The "data" field without outputs was marked as "required" in the help docs when using bitcoin-cli. This field when left off worked as an intended optional OP_RETURN. closes #27828.
Motivation: It is hard to understand that "data" is actually optional for commands like `createpsbt` and `walletcreatefundedpsbt`.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull reques
...
💬 sipa commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577106762)
I get that this is confusing, but this change isn't correct.
The field is required when the object is present (as in `[{"address": amount, ..., {}]` would not be valid). What's optional is the object, which is at a higher level.
Maybe it's worth elaborating in the prose text above?
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577106762)
I get that this is confusing, but this change isn't correct.
The field is required when the object is present (as in `[{"address": amount, ..., {}]` would not be valid). What's optional is the object, which is at a higher level.
Maybe it's worth elaborating in the prose text above?
💬 instagibbs commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1577106769)
> I think if sensitive relay is used this can be made more aggressive and/or increase the number of peers we broadcast to. E.g. after 3 minutes broadcast to 10 peers.
I think for first cut matching current behavior is probably best. If Tor is being sybiled, I don't think hammering things even harder is going to fix much. Time-sensitive users should probably just avoid using this feature altogether, at least until there is some sort of fallback mechanism introduced.
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1577106769)
> I think if sensitive relay is used this can be made more aggressive and/or increase the number of peers we broadcast to. E.g. after 3 minutes broadcast to 10 peers.
I think for first cut matching current behavior is probably best. If Tor is being sybiled, I don't think hammering things even harder is going to fix much. Time-sensitive users should probably just avoid using this feature altogether, at least until there is some sort of fallback mechanism introduced.
💬 miketwenty1 commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577132445)
> I get that this is confusing, but this change isn't correct.
>
> The `"data"` field itself is required when the object is present (as in `[{"address": amount, ..., {}]` would not be valid). What's optional is the object (the `{ ... }` around it), which is at a higher level.
>
> Maybe it's worth elaborating in the prose text above?
Understood you're saying the change isn't correct. How would someone properly indicate the "data" is optional if not specifying it as optional? The parent o
...
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577132445)
> I get that this is confusing, but this change isn't correct.
>
> The `"data"` field itself is required when the object is present (as in `[{"address": amount, ..., {}]` would not be valid). What's optional is the object (the `{ ... }` around it), which is at a higher level.
>
> Maybe it's worth elaborating in the prose text above?
Understood you're saying the change isn't correct. How would someone properly indicate the "data" is optional if not specifying it as optional? The parent o
...
💬 sipa commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577142430)
The `"data": ...` part cannot be left out. It's the `{"data": ...}` that can be left out.
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577142430)
The `"data": ...` part cannot be left out. It's the `{"data": ...}` that can be left out.
💬 miketwenty1 commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577147866)
> The `"data": ...` part cannot be left out. It's the `{"data": ...}` that can be left out.
Let me rephrase my question:
How would someone know `{"data" ...}` is optional if not specifying it as optional? If every field is optional within an obj maybe it's implied the obj itself is optional?
Does there need to be a new idea of specifying whether the (json object) itself is optional?
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577147866)
> The `"data": ...` part cannot be left out. It's the `{"data": ...}` that can be left out.
Let me rephrase my question:
How would someone know `{"data" ...}` is optional if not specifying it as optional? If every field is optional within an obj maybe it's implied the obj itself is optional?
Does there need to be a new idea of specifying whether the (json object) itself is optional?