💬 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.

🚀 fanquake merged a pull request: "doc: remove Tor link & generalize onion getnodeaddresses RPC"
(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
...

💬 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
```

📝 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
...

💬 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.

💬 achow101 commented on pull request "walletdb: Add PrefixCursor":
(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
...

💬 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.

💬 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.

💬 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?

💬 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
...

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1213346408)
Ended up not adding this change to not modify `AbortShutdown()`.

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1572294451)
Feedback tackled. Thanks TheCharlatan!. Changes:

* Moved ThreadImport ABC error to use `AbortNode`. Per [comment](https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1206967261).
* And moved `AbortNode` code below `StartShutdown` so it can use the introduced static function instead of having to add a public `StartErrorShutdown` function that is only used by the .cpp file and not externally.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213371595)
Good observation. If both Tor and I2P are unreachable then `m_sensitive_relay_connections_to_open` should always be zero. Inside `PeerManagerImpl::ScheduleLocalTxForRelay()` `UseSensitiveRelay()` would return `false` and `m_connman.ScheduleSensitiveRelayConnections()` which increments the counter will never be called.

Now, that is the logic of some code far away, in another file. I did not want to rely on it because if we enter here and both are unreachable, then it will end up in an infinite
...

💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1572348643)
Updated 5559ad2c69ef82c18843b9214e5ba3974834a1ae -> 2cd28e9fef5dd743bcd70025196ee311fcfdcae4 ([`pr/nonly.16`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.16) -> [`pr/nonly.17`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.17), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nonly.16..pr/nonly.17)) with suggested test and conversion table fixes. I also changed the code to use actual position of the options parameters as originally suggested, instead of -1 values.

💬 vasild commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#issuecomment-1572354107)
Concept ACK, I guess we can forget about Torv2 and completely drop support about it from everywhere as if it never existed. The Tor network does not support those anymore.

📝 ryanofsky opened a pull request: "wallet: Add tracing for sqlite statements"
(https://github.com/bitcoin/bitcoin/pull/27801)
I found sqlite tracing was useful for debugging a test in #27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options.

👋 ryanofsky's pull request is ready for review: "streams: Drop confusing DataStream::Serialize method and << operator"
(https://github.com/bitcoin/bitcoin/pull/27800)

💬 vasild commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1213400231)
This could collide with another port randomly assigned by the testing framework (even if it looks like that it currently cannot, it may do in the future) and also could collide with another instance of this test run in parallel. Better use `p2p_port()`.