💬 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?
💬 mzumsande commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1577161538)
> Needs rebase?
yes. Rebased due to silent conflict with #26261.
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1577161538)
> Needs rebase?
yes. Rebased due to silent conflict with #26261.
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1577177916)
> Tor-to-IPv4 exit nodes,
I was more asking about this, not as much from a vulnerability standpoint but more like, the bottleneck created by all TXs going through a specific subset of network nodes first. I think your envelope math is reassuring though - I didn't realize there were so many Tor nodes already
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1577177916)
> Tor-to-IPv4 exit nodes,
I was more asking about this, not as much from a vulnerability standpoint but more like, the bottleneck created by all TXs going through a specific subset of network nodes first. I think your envelope math is reassuring though - I didn't realize there were so many Tor nodes already
💬 mzumsande 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_r1218371741)
> Wouldn't that be broken if we ever add a parallel headers download process?
> Which IIRC we don't have because they are downloaded from a single peer at time. But if we implement it, we could get orphan block indexes pretty easily.
Yes, but only we had a very naive parallel headers download process. I would imagine that if we ever parallelized headers download, we'd take care not to accept orphaned block indexes into our main block index (for DoS reasons) but would keep those headers in so
...
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1218371741)
> Wouldn't that be broken if we ever add a parallel headers download process?
> Which IIRC we don't have because they are downloaded from a single peer at time. But if we implement it, we could get orphan block indexes pretty easily.
Yes, but only we had a very naive parallel headers download process. I would imagine that if we ever parallelized headers download, we'd take care not to accept orphaned block indexes into our main block index (for DoS reasons) but would keep those headers in so
...
💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1577187405)
> How do you see the division of labor between `ChainstateManager` and `BlockManager`? The `ChainState` doc currently says:
>
> > Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in `BlockManager`.
I think of `BlockManager` as being a generic backend database of block (and undo) data, while any validation-related logic should live in `Chainstate` (if it relies on chainstate-specific inf
...
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1577187405)
> How do you see the division of labor between `ChainstateManager` and `BlockManager`? The `ChainState` doc currently says:
>
> > Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in `BlockManager`.
I think of `BlockManager` as being a generic backend database of block (and undo) data, while any validation-related logic should live in `Chainstate` (if it relies on chainstate-specific inf
...
💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1577188477)
> How do you see the division of labor between `ChainstateManager` and `BlockManager`? The `ChainState` doc currently says:
>
> > Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in `BlockManager`.
I think of `BlockManager` as being a generic backend database of block (and undo) data, while any validation-related logic should live in `Chainstate` (if it relies on chainstate-specific inf
...
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1577188477)
> How do you see the division of labor between `ChainstateManager` and `BlockManager`? The `ChainState` doc currently says:
>
> > Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in `BlockManager`.
I think of `BlockManager` as being a generic backend database of block (and undo) data, while any validation-related logic should live in `Chainstate` (if it relies on chainstate-specific inf
...
💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1218382890)
This change would make `feature_proxy.py` test fail because the error would be `"Invalid port..."` which is misleading if the real problem is (for example) a too-long file path, which will get caught by `IsUnixSocketPath()` on L1370 and described as `"Invalid -proxy address or hostname..."`. So I think it makes more sense to keep this line as-is: if we think the user is trying to set a UNIX socket path, we just ignore the invalid port check.
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1218382890)
This change would make `feature_proxy.py` test fail because the error would be `"Invalid port..."` which is misleading if the real problem is (for example) a too-long file path, which will get caught by `IsUnixSocketPath()` on L1370 and described as `"Invalid -proxy address or hostname..."`. So I think it makes more sense to keep this line as-is: if we think the user is trying to set a UNIX socket path, we just ignore the invalid port check.
💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1577201958)
@vasild thanks for the excellent detailed review! All comments addressed except where noted.
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1577201958)
@vasild thanks for the excellent detailed review! All comments addressed except where noted.