✅ fanquake closed a pull request: "rpc, net: add erlay status in `getpeerinfo`"
(https://github.com/bitcoin/bitcoin/pull/27797)
(https://github.com/bitcoin/bitcoin/pull/27797)
💬 MarnixCroes commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#issuecomment-1571677493)
@jonatack thank you! I've applied your suggestions
(https://github.com/bitcoin/bitcoin/pull/27719#issuecomment-1571677493)
@jonatack thank you! I've applied your suggestions
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1571690382)
`>>` This is ready for review and testing `<<`
> ... if the transaction is sent along with the potentially uniquely identifying UA comment, or other fingerprints in the version message. Maybe it makes sense to define a static version message ...
@MarcoFalke, I went with a [completely static `VERSION` message](https://github.com/vasild/bitcoin/blob/b73810f80917dd882d6161f6892f10f8c46a8193/src/net_processing.cpp#L1439-L1446)
> ... Perhaps it's worth mimicking `bitcoin-submittx`
@sipa,
...
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1571690382)
`>>` This is ready for review and testing `<<`
> ... if the transaction is sent along with the potentially uniquely identifying UA comment, or other fingerprints in the version message. Maybe it makes sense to define a static version message ...
@MarcoFalke, I went with a [completely static `VERSION` message](https://github.com/vasild/bitcoin/blob/b73810f80917dd882d6161f6892f10f8c46a8193/src/net_processing.cpp#L1439-L1446)
> ... Perhaps it's worth mimicking `bitcoin-submittx`
@sipa,
...
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1212891740)
This, together with the other change from commit 0ec7b979e7a9d4c30a97c39c2a64768d7c5662b1 `net_processing: omit unbroadcast txs from replies to GETDATA and MEMPOOL` alter the behavior even if sensitive relay is not used (e.g. disabled or Tor and I2P not reachable).
I think it is beneficial in that case too, but is not the purpose of this PR to improve that. Should this be guarded by `if (UseSensitiveRelay())`?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1212891740)
This, together with the other change from commit 0ec7b979e7a9d4c30a97c39c2a64768d7c5662b1 `net_processing: omit unbroadcast txs from replies to GETDATA and MEMPOOL` alter the behavior even if sensitive relay is not used (e.g. disabled or Tor and I2P not reachable).
I think it is beneficial in that case too, but is not the purpose of this PR to improve that. Should this be guarded by `if (UseSensitiveRelay())`?
👋 vasild's pull request is ready for review: "Relay own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/27509)
(https://github.com/bitcoin/bitcoin/pull/27509)
💬 fanquake commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1212894727)
`These results, as well as those of -addrinfo, are filtered for quality and recency.` - can you drop this. It's not clear why we need to mention an implemention detail of `-addrinfo` here.
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1212894727)
`These results, as well as those of -addrinfo, are filtered for quality and recency.` - can you drop this. It's not clear why we need to mention an implemention detail of `-addrinfo` here.
💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212736489)
ee6e289:
```suggestion
def __rsub__(self, a):
```
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212736489)
ee6e289:
```suggestion
def __rsub__(self, a):
```
💬 stratospher commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212950016)
> The way FE values are represented inside the class is an unobservable implementation detail, so calling int(FE) doesn't "update" the FE object in any observable way - it just makes future calls more efficient.
thinking of it this way makes sense. thanks!
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1212950016)
> The way FE values are represented inside the class is an unobservable implementation detail, so calling int(FE) doesn't "update" the FE object in any observable way - it just makes future calls more efficient.
thinking of it this way makes sense. thanks!
💬 brandonpille commented on issue "rpc: Allow importing wallets by data instead of by filename":
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1571814230)
No it's ok
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1571814230)
No it's ok
✅ fanquake closed an issue: "rpc: Allow importing wallets by data instead of by filename"
(https://github.com/bitcoin/bitcoin/issues/27597)
(https://github.com/bitcoin/bitcoin/issues/27597)
💬 MarnixCroes commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1212994429)
done
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1212994429)
done
💬 theuni commented on pull request "depends: modernize clang flags for Darwin":
(https://github.com/bitcoin/bitcoin/pull/27798#discussion_r1213032495)
ffs...
(https://github.com/bitcoin/bitcoin/pull/27798#discussion_r1213032495)
ffs...
💬 jonatack commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213064407)
> It's not clear why we need to mention an implemention detail of `-addrinfo` here.
That is how RPC `getnodeaddresses` works as well -- see its help.
Would suggest either reverting to the previous push, or replacing this paragraph in tor/i2p/cjdns.md with something like the following:
You can use the `getnodeaddresses` RPC to fetch a number of onion peers known to your node; run `bitcoin-cli help getnodeaddresses` for details.
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213064407)
> It's not clear why we need to mention an implemention detail of `-addrinfo` here.
That is how RPC `getnodeaddresses` works as well -- see its help.
Would suggest either reverting to the previous push, or replacing this paragraph in tor/i2p/cjdns.md with something like the following:
You can use the `getnodeaddresses` RPC to fetch a number of onion peers known to your node; run `bitcoin-cli help getnodeaddresses` for details.
💬 jonatack commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213069877)
Now that the link has been removed, please update the PR title and description from "fix" to "remove".
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213069877)
Now that the link has been removed, please update the PR title and description from "fix" to "remove".
💬 glozow commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1571967192)
> Because a -fallback fee already exists. Why can't it exist for other use cases?
Node and wallet are 2 separate components; it doesn't makes sense for a wallet option to bleed into node behavior. Another example: the wallet setting `-walletrejectlongchains=0` does not mean the mempool should also accept long chains of transactions, and the existence of that wallet option does not mean we should also add that option to the node. If inheritance exists, the wallet's options should be informed b
...
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1571967192)
> Because a -fallback fee already exists. Why can't it exist for other use cases?
Node and wallet are 2 separate components; it doesn't makes sense for a wallet option to bleed into node behavior. Another example: the wallet setting `-walletrejectlongchains=0` does not mean the mempool should also accept long chains of transactions, and the existence of that wallet option does not mean we should also add that option to the node. If inheritance exists, the wallet's options should be informed b
...
💬 MarcoFalke commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1571998659)
Side note: I tried centos and it also failed with the same error as debian. Commit:
<details><summary>diff</summary>
```diff
commit 57903b154ec1dc5d86f174d311e9875bbf0c4106
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date: Thu Jun 1 13:48:41 2023 +0200
ci: Use centos to work around fuzz msan debian bug
diff --git a/ci/test/00_setup_env_native_fuzz_with_msan.sh b/ci/test/00_setup_env_native_fuzz_with_msan.sh
index dd694f818c..5aca7a9be9 100755
--- a/ci/test/00_setu
...
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1571998659)
Side note: I tried centos and it also failed with the same error as debian. Commit:
<details><summary>diff</summary>
```diff
commit 57903b154ec1dc5d86f174d311e9875bbf0c4106
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date: Thu Jun 1 13:48:41 2023 +0200
ci: Use centos to work around fuzz msan debian bug
diff --git a/ci/test/00_setup_env_native_fuzz_with_msan.sh b/ci/test/00_setup_env_native_fuzz_with_msan.sh
index dd694f818c..5aca7a9be9 100755
--- a/ci/test/00_setu
...
💬 fanquake commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213129896)
> You can use the getnodeaddresses RPC to fetch a number of onion peers known to your node; run bitcoin-cli help getnodeaddresses for details.
If anything, this seems better than reverting to the previous text.
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213129896)
> You can use the getnodeaddresses RPC to fetch a number of onion peers known to your node; run bitcoin-cli help getnodeaddresses for details.
If anything, this seems better than reverting to the previous text.
💬 jonatack commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213130776)
Agree.
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1213130776)
Agree.
💬 dergoegge commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1572044611)
utACK 5763b232e6e6a0f72d046f8aa322b39328be135b
Looking forward to green check marks on qa-assets
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1572044611)
utACK 5763b232e6e6a0f72d046f8aa322b39328be135b
Looking forward to green check marks on qa-assets
💬 ryanofsky commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213206571)
> Is the same not true for span itself?
No because `Span<char>` is fixed length and can be deserialized, while `DataStream` is variable length and can't be deserialized.
Reasons why I think it would be good to get rid of `DataStream::Serialize` method:
- `DataStream` has no `Unserialize` method and no reasonable way of adding one that would be consistent with the existing `Serialize` implementation.
- The `Serialize` method has surprising behavior. If `DataStream` was going to have a
...
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213206571)
> Is the same not true for span itself?
No because `Span<char>` is fixed length and can be deserialized, while `DataStream` is variable length and can't be deserialized.
Reasons why I think it would be good to get rid of `DataStream::Serialize` method:
- `DataStream` has no `Unserialize` method and no reasonable way of adding one that would be consistent with the existing `Serialize` implementation.
- The `Serialize` method has surprising behavior. If `DataStream` was going to have a
...