💬 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
...
💬 darosior commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1572121111)
> When scripts are decoded into ASM, two different integers can be displayed identically, with one as hex and the other as decimal.
It's not two different integers, it's that integers (which all <=4 bytes pushes are assumed to be) are displayed in decimal and all other pushes are displayed as hexadecimal.
I wonder why we even try to detect and print numbers differently (and even more so in a different base!), just print all pushes in hex and there is no ambiguity anymore?
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1572121111)
> When scripts are decoded into ASM, two different integers can be displayed identically, with one as hex and the other as decimal.
It's not two different integers, it's that integers (which all <=4 bytes pushes are assumed to be) are displayed in decimal and all other pushes are displayed as hexadecimal.
I wonder why we even try to detect and print numbers differently (and even more so in a different base!), just print all pushes in hex and there is no ambiguity anymore?
💬 instagibbs commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1572135299)
> Approach review is very welcome on this PR
didn't real whole OP until now. In my not expert opinion, the orphan handling seems reasonable, and I think it makes to open it ASAP to take it out of draft
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1572135299)
> Approach review is very welcome on this PR
didn't real whole OP until now. In my not expert opinion, the orphan handling seems reasonable, and I think it makes to open it ASAP to take it out of draft
👍 fanquake approved a pull request: "doc: remove Tor link & generalize onion getnodeaddresses RPC"
(https://github.com/bitcoin/bitcoin/pull/27719#pullrequestreview-1455587440)
ACK 6fce5ddc17ac9d1e07849f92088ea3f7cfcafe26
(https://github.com/bitcoin/bitcoin/pull/27719#pullrequestreview-1455587440)
ACK 6fce5ddc17ac9d1e07849f92088ea3f7cfcafe26
💬 MatthewLM commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1572151288)
They are two different numbers. One is `0x1112131415` and the other is `0x57c74942`. In ASM, they are displayed the same. Using `0x` before hex is consistent with hex literals elsewhere and would allow for decimals to be displayed including `-1` for `OP_1NEGATE`.
Displaying all numbers as hex is an option. One problem is that they are given as little-endian which could be confusing for some wishing to interpret them. Off the top of my memory, the scripting interpreter treats 32-bit numbers ar
...
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1572151288)
They are two different numbers. One is `0x1112131415` and the other is `0x57c74942`. In ASM, they are displayed the same. Using `0x` before hex is consistent with hex literals elsewhere and would allow for decimals to be displayed including `-1` for `OP_1NEGATE`.
Displaying all numbers as hex is an option. One problem is that they are given as little-endian which could be confusing for some wishing to interpret them. Off the top of my memory, the scripting interpreter treats 32-bit numbers ar
...
💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1572153753)
> It looks like the conversion table is missing the arguments that take amounts; these also need to be converted.
Thank you! Will apply patch. I'm surprised amounts need to be converted, though. I thought AmountFromValue handled both numbers and strings so strings would be fine.
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1572153753)
> It looks like the conversion table is missing the arguments that take amounts; these also need to be converted.
Thank you! Will apply patch. I'm surprised amounts need to be converted, though. I thought AmountFromValue handled both numbers and strings so strings would be fine.
🚀 fanquake merged a pull request: "doc: remove Tor link & generalize onion getnodeaddresses RPC"
(https://github.com/bitcoin/bitcoin/pull/27719)
(https://github.com/bitcoin/bitcoin/pull/27719)
💬 brunoerg commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213240959)
In e310a0d285db0b82b0c201a56f92a7bfe4663226, you added these verifications, I think it would be useful if you add coverage for them in b6fd6b525d3ee4d89331dc04d86f854cb1decccd
Suggestion (did it to test e310a0d285db0b82b0c201a56f92a7bfe4663226):
```diff
diff --git a/test/functional/p2p_local_tx_relay.py b/test/functional/p2p_local_tx_relay.py
index 0b115352d..46215110e 100755
--- a/test/functional/p2p_local_tx_relay.py
+++ b/test/functional/p2p_local_tx_relay.py
@@ -140,12 +140,12 @@ cl
...
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213240959)
In e310a0d285db0b82b0c201a56f92a7bfe4663226, you added these verifications, I think it would be useful if you add coverage for them in b6fd6b525d3ee4d89331dc04d86f854cb1decccd
Suggestion (did it to test e310a0d285db0b82b0c201a56f92a7bfe4663226):
```diff
diff --git a/test/functional/p2p_local_tx_relay.py b/test/functional/p2p_local_tx_relay.py
index 0b115352d..46215110e 100755
--- a/test/functional/p2p_local_tx_relay.py
+++ b/test/functional/p2p_local_tx_relay.py
@@ -140,12 +140,12 @@ cl
...
💬 darosior commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1572163402)
Sure, i misspoke. I just meant:
```python
>>> int.from_bytes(bytes.fromhex("57c74942"), "little")
1112131415
```
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1572163402)
Sure, i misspoke. I just meant:
```python
>>> int.from_bytes(bytes.fromhex("57c74942"), "little")
1112131415
```
📝 ryanofsky opened a pull request: "streams: Drop confusing DataStream::Serialize method and << operator"
(https://github.com/bitcoin/bitcoin/pull/27800)
DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes.
Having this inconsistency is not necessary and could be confusing (see https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030) so this
...
(https://github.com/bitcoin/bitcoin/pull/27800)
DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes.
Having this inconsistency is not necessary and could be confusing (see https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030) so this
...
💬 achow101 commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213277212)
I think @MarcoFalke's point was that `Span`s are serialized without length prefixes, so this function could just pass `key` and `value` directly to `DatabaseBatch::Write` rather than going through a `DataStream`, as long as there is a `Serialize` method for `Span<const std::byte>`. This seems like a more intuitive solution, so I've implemented that.
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213277212)
I think @MarcoFalke's point was that `Span`s are serialized without length prefixes, so this function could just pass `key` and `value` directly to `DatabaseBatch::Write` rather than going through a `DataStream`, as long as there is a `Serialize` method for `Span<const std::byte>`. This seems like a more intuitive solution, so I've implemented that.
💬 achow101 commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213277344)
Done
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213277344)
Done
💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1572205313)
Offline feedback suggested I clarify what I mean by "approach feedback welcome" before "I open separate PRs."
This is a large project, and the first few p2p commits essentially define the interface. I'd like to get rough consensus on approach before we start looking at code details and merging PRs, because I believe it will help us "get useful stuff in" faster and avoid premature optimizations.
Here are some approach questions I think we should answer before diving in:
1. Does the propose
...
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1572205313)
Offline feedback suggested I clarify what I mean by "approach feedback welcome" before "I open separate PRs."
This is a large project, and the first few p2p commits essentially define the interface. I'd like to get rough consensus on approach before we start looking at code details and merging PRs, because I believe it will help us "get useful stuff in" faster and avoid premature optimizations.
Here are some approach questions I think we should answer before diving in:
1. Does the propose
...
💬 sipa commented on pull request "streams: Drop confusing DataStream::Serialize method and << operator":
(https://github.com/bitcoin/bitcoin/pull/27800#issuecomment-1572208374)
Concept ACK. I agree this operator is confusing and inconsistent with the properties you'd expect from serialization.
(https://github.com/bitcoin/bitcoin/pull/27800#issuecomment-1572208374)
Concept ACK. I agree this operator is confusing and inconsistent with the properties you'd expect from serialization.
💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1572216378)
> I'm surprised amounts need to be converted, though
It actually may just be the test that checks the conversion table that requires it since `dump_all_command_conversions` marks amounts as needed to be converted. However converting amounts is also consistent with all of the other RPCs that accept amounts as arguments, even prior to that test being added.
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1572216378)
> I'm surprised amounts need to be converted, though
It actually may just be the test that checks the conversion table that requires it since `dump_all_command_conversions` marks amounts as needed to be converted. However converting amounts is also consistent with all of the other RPCs that accept amounts as arguments, even prior to that test being added.
💬 brunoerg commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213295906)
In 31dd230738c43a529c63331eb23135e42d083e8f: At this point (creation of `open_sensitive_relay`), is there any possibility of both tor and i2p to be unreachable?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213295906)
In 31dd230738c43a529c63331eb23135e42d083e8f: At this point (creation of `open_sensitive_relay`), is there any possibility of both tor and i2p to be unreachable?
💬 vasild commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1213318442)
I agree with what you say above - `whitebind` on publicly accessible address:port with this new permission sounds bad. Similar with `whitelist` - if a range is used.
This PR currently expands the semantic of the `noban` permission. This will affect existent setups that already use it. Would it make sense to introduce a new permisson, separate from `noban`? I mean - now if somebody is running `-whitebind=noban@publicaddr:port` then a bad actor could cause harm on `master`, but even more harm w
...
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1213318442)
I agree with what you say above - `whitebind` on publicly accessible address:port with this new permission sounds bad. Similar with `whitelist` - if a range is used.
This PR currently expands the semantic of the `noban` permission. This will affect existent setups that already use it. Would it make sense to introduce a new permisson, separate from `noban`? I mean - now if somebody is running `-whitebind=noban@publicaddr:port` then a bad actor could cause harm on `master`, but even more harm w
...