💬 theStack commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1218496768)
Have no strong opinion on that either, just wanted to make sure that there wasn't some code using `modified_fee` field around that was maybe forgotten to be pushed or sth alike.

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1218526503)
nice simplification! incorporated in latest push

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1218543773)
yeah, I see what you're saying - that we can avoid iterating through all peers when we are only running on one network.

I've implemented a version of your suggestion in https://github.com/amitiuttarwar/bitcoin/commit/1805698096ae29e8dd6ecd1be9348a04f612602c. I changed the function implementation and also invoked from the additional call site in `MaybePickPreferredNetwork`. It seems viable.

I haven't pushed it here yet because I'm still trying to figure out a slightly cleaner solution. I n
...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1577424366)
thanks for the review @brunoerg ! latest push incorporated fuzz test & `ThreadOpenConnections` simplification. I'm still working on the other feedback (as explained above).

one question about your suggested patch in the fuzz test:
`const Network net{fuzzed_data_provider.PickValueInArray({Network::NET_IPV4, Network::NET_IPV6, Network::NET_INTERNAL, Network::NET_ONION})};`
is there a reason to select from the subset of networks? in the patch I pushed up, I also added the other ones. I saw t
...

👍 ryanofsky approved a pull request: "wallet, tests: Expand and test when the blank wallet flag should be un/set"
(https://github.com/bitcoin/bitcoin/pull/25634#pullrequestreview-1463212375)
Code review ACK 7baa0971527ba3c8d87b82c7a2a4eabf6abbb2b7, but only lightly reviewed test. I suggested some documentation updates, but these are obviously not critical.

This change does add good test coverage and seems like an improvement over the status quo. I think I'm not really clear on the benefits of setting the blank flag in createwallet when the disable private keys flag is set, though. I wonder if we considered changing the GUI behavior to match RPC behavior instead of vice versa, and
...

💬 ryanofsky commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1218479148)
In commit "rpc, wallet: Set blank when disable private keys in createwallet" (5f3c64c3fd845228227e28b7cefa3e6c5c0ba7ef)

The comment is saying "We want to set BLANK when DISABLE_PRIVATE_KEYS" without saying why.

I think I would make the comment try to explain why to make the code understandable. Would suggest something like:

* By default, try to set the BLANK flag when the DISABLE_PRIVATE_KEYS flag is set, to reflect the fact that the new wallet will not contain any keys or scripts or d
...

💬 ryanofsky commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1218554571)
Change to unset the blank flag when writing a descriptor is now reverted, so I think this makes BLANK flag documentation misleading, and could be fixed with:

```diff
--- a/src/wallet/walletutil.h
+++ b/src/wallet/walletutil.h
@@ -53,10 +53,18 @@ enum WalletFlags : uint64_t {
//! Flag set when a wallet contains no HD seed and no private keys, scripts,
//! addresses, and other watch only things, and is therefore "blank."
//!
- //! The only function this flag serves is t
...

💬 ryanofsky commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1218522240)
In commit "tests: Test that the blank wallet flag is un/set as expected" (7baa0971527ba3c8d87b82c7a2a4eabf6abbb2b7)

Would be nice to add the test earlier as the second commit or part of the first commit, so it would be easily possible to see how other commits in the PR affect the test cases

💬 ryanofsky commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1218493457)
In commit "wallet: Ensure that the blank wallet flag is unset after imports" (215694947a5dce9e2f2617b7ee688256366d011e)

This commit doesn't seem to have any test coverage. Maybe that's ok because the change doesn't really effect visible behavior. But I was wondering if this hard to test for some other reason

💬 miketwenty1 commented on issue "bitcoin-cli output help text - (json object) not utilizing RPCArg::Optional value":
(https://github.com/bitcoin/bitcoin/issues/27828#issuecomment-1577451920)
Got clarity that 1 of the 2 fields is required either address or data. Maybe just a clean up of the description is fine.

💬 miketwenty1 commented on pull request "DRAFT: rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577485584)
@sipa, edited the commit. Something like this look good?

💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1218613588)
This includes commits to drop `mapRelay` now (cf #27625) so shouldn't be broken anymore.

💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1218614297)
This isn't renamed anymore, instead `GetEntryTime()` is added for the steady clock time tracking mempool entry. (Happy to rename it to something better if someone has a suggestion)

💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1218614612)
Switched to steady clock.

💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1218615193)
Entry time is no longer saved or loaded, so this is no longer relevant.

💬 sipa commented on pull request "DRAFT: rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577505316)
ACK 30acffa32023838d7b0c3f48d2a3688fae15490a

🤔 mzumsande reviewed a pull request: "Relay own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/27509#pullrequestreview-1463428381)
reviewed just the first few commits so far

💬 mzumsande commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218588419)
typo: sensitive-relay (extra "t")

💬 mzumsande commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218597944)
If this bypasses `-maxconnections`, could we run into trouble if we don't have sufficient file descriptors (see `RaiseFileDescriptorLimit()` call in init)?

💬 mzumsande commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218585985)
There could be a race with the `torcontrol` thread, which may set `NET_ONION` to reachable before this line is encountered or only later.