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

💬 mzumsande commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218631408)
I'm a bit worried about this priority: What would happen if we, for some reason, aren't able to make successful sensitive relay connections (e.g. we think that we can connect to Tor but misconfigured something, or maybe we can connect but don't have any addresses for these networks in our addrman).
Wouldn't we try again and again (since this is the first priority and `m_sensitive_relay_connections_to_open` is never reduced) and never attempt to make an outbound connection of another type?

📝 Brotcrunsher opened a pull request: "Supporting parameter "h" and "?" in -netinfo."
(https://github.com/bitcoin/bitcoin/pull/27830)
The main -help argument supports these two as well, so we might as well behave the same here.

💬 ariard commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1577688541)
Note for the clarity, I think effectively both a loosening (e.g nVersion=3 where TX_MAX_STANDARD_VERSION allows nVersion=3 transaction propagation) and a tightening (e.g some nSequence field usage being banned from propagation) can be used to commit a double-spend.

With a loosening, if the victim full-node is not upgraded, they won't accept your new nVersion=3 transactions, and you can feed the rest of the upgraded network with your nVersion=3 double-spend.

With a tightening, if the victim
...

💬 0xB10C commented on issue "Duplicate coinbase transaction space reservation in CreateNewBlock":
(https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-1577702096)
> And actually it looks like ALL of F2Pool's recent blocks are actually > 3,996,000 in weight. They may be the only pool using their own software to create block templates?

I'm fairly certain they use Bitcoin Core. You can control the max block weight with `-blockmaxweight` (default is 3996000 WU). Also, F2Pool is known for running with custom patches on top of Bitcoin Core to maximize what they can: e.g. they reduced the number of sigops reserved for the coinbase. This [bit them twice](https
...