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

💬 pinheadmz commented on issue "Duplicate coinbase transaction space reservation in CreateNewBlock":
(https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-1577730448)
@0xB10C thanks for these great insights. What do you think about the issue? The `4000` is reserved in two places:

https://github.com/bitcoin/bitcoin/blob/f4a8269dfc144cc918570bdb870aa5143a11c1fe/src/node/miner.cpp#L97

https://github.com/bitcoin/bitcoin/blob/f4a8269dfc144cc918570bdb870aa5143a11c1fe/src/policy/policy.h#L23

Should we remove one of these?

💬 ajtowns commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1577762400)
> 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.

Any mempool acceptance policy difference between two nodes can be used to prevent your node from seeing transactions the other node accepts. If you outright reject tx A, then spending an output from that tx will suffice. If you a
...

💬 jarolrod commented on pull request "guix: Update `python-lief` package to 0.13.1":
(https://github.com/bitcoin/bitcoin/pull/27813#issuecomment-1577862296)
Guix hashes

```
f3006576f4882414f9bbe9e37dbf54567bf24cff5252e487260a79d7f411c0bd guix-build-e3792a660abf/output/aarch64-linux-gnu/SHA256SUMS.part
61e90a272ab6d534ade60c46da90809f5d03017d31fed4b24225780db8c4aa4d guix-build-e3792a660abf/output/aarch64-linux-gnu/bitcoin-e3792a660abf-aarch64-linux-gnu-debug.tar.gz
a6e23c73e89076f5cb8c712effd6dd5796fb1e9bfd720b9310b829128e7a94f1 guix-build-e3792a660abf/output/aarch64-linux-gnu/bitcoin-e3792a660abf-aarch64-linux-gnu.tar.gz
8531ad1072f660de83
...