🤔 instagibbs reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2017442665)
reviewed through 55b1280c52af81aa6ea0860799fa16da49f51447

Looks good, have a suggestion for a test catching the `Assume()` issue(which does actually get hit in mainnet pretty fast with debug on)

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576365857)
is there a constant we can put here?

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576479584)
regression test for the `Assume()` that was hit? And unsetting the mocktime to not have interference in subtests...
```
diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py
index 2eaa2a0a79..f3a741498b 100755
--- a/test/functional/p2p_opportunistic_1p1c.py
+++ b/test/functional/p2p_opportunistic_1p1c.py
@@ -291,58 +291,97 @@ class PackageRelayTest(BitcoinTestFramework):
child_bumping = self.wallet_nonsegwit.create_self_transfer_multi(
...

👍 instagibbs approved a pull request: "feefrac: avoid explicitly computing diagram; compare based on chunks"
(https://github.com/bitcoin/bitcoin/pull/29757#pullrequestreview-2017766616)
reACK https://github.com/bitcoin/bitcoin/pull/29757/commits/b22901dfa9cc3af94bf13163a28300eb1ab25b22

🤔 ismaelsadeeq reviewed a pull request: "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`"
(https://github.com/bitcoin/bitcoin/pull/29939#pullrequestreview-2017794758)
Concept ACK, having a separate util for mempool is nice, all mempool helpers will live there.

> I'm still not sure if a generic word like "tag" is the right term for what this tries to achieve, happy to pick up better suggestions. Also, maybe passing a tag name is overkill and a boolean flag like "random_output_script" is sufficient?

I don't feel strongly on this but I like the second idea of just boolean flag.

💬 achow101 commented on pull request "util: remove unused cpp-subprocess options":
(https://github.com/bitcoin/bitcoin/pull/29865#issuecomment-2072929586)
ACK 13adbf733f09c73c3cf0025d94c52f9cec5dba3b

💬 Sjors commented on pull request "[DO NOT MERGE] testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2072935478)
> I am not sure how the change here is making a difference?

If it depends on the difficulty being 1 rather 1 million, that would make a difference. The two people who brought it can definitely recompile, but maybe there's a better solution - maybe just a startup flag to override the minimum difficulty?

🚀 achow101 merged a pull request: "util: remove unused cpp-subprocess options"
(https://github.com/bitcoin/bitcoin/pull/29865)

💬 maflcko commented on pull request "[DO NOT MERGE] testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2072953283)
> maybe just a startup flag to override the minimum difficulty?

I don't think consensus rules of remote nodes can be affected by a local startup flag (or re-compilation).

If someone wanted to create a block locally only, they could use regtest.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2072977453)
`9297437af2...cc760207b8`: rebase and address suggestions:

* Give a startup warning if `-privatebroadcast=1`, `-proxyrandomize=0` and the Tor network is reachable (i.e. we will use Tor for private broadcast).
* Enforce `-walletbroadcast=0` if `-privatebroadcast=1` because it would be confusing to have the wallet do the traditional broadcast while the `sendrawtransaction` RPC does a private broadcast. Furthermore if a wallet transaction is sent via `sendrawtransaction` and ends up in the memp
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1576618058)
Right, removed the conflicting address and reduced the logging to log only if adding to the addrman fails.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1576619537)
I added broadcast to IPv4 and IPv6 peers through the Tor proxy. Thanks!

👋 hernanmarino's pull request is ready for review: "test: add missing tests for Assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/29428)

👋 hebasto's pull request is ready for review: "refactor: Rename `subprocess.hpp` to follow our header name conventions"
(https://github.com/bitcoin/bitcoin/pull/29910)

💬 hernanmarino commented on pull request "test: add missing tests for Assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29428#issuecomment-2072984921)
> Are you still working on this?

I haven´t been able to think of / work on other tests to add, but the current commit test is ready for review, so i am taking the PR out of Draft status.

💬 hebasto commented on pull request "refactor: Rename `subprocess.hpp` to follow our header name conventions":
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2072985412)
Rebase and undrafted.

💬 hebasto commented on pull request "util: remove unused cpp-subprocess options":
(https://github.com/bitcoin/bitcoin/pull/29865#issuecomment-2072986554)
https://github.com/bitcoin/bitcoin/pull/29910 is the next one :)

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2072989163)
> Avoiding already connected peers would work around this, but perhaps it's sufficient just to warn on startup if privatebroadcast=1 and proxyrandomize=0

In `master` we already avoid connecting to an already connected address, regardless of the connection type:

https://github.com/bitcoin/bitcoin/blob/256e1703197fdddd78bc6d659431cd0fc3b63cde/src/net.cpp#L2862

I added a startup warning anyway.



> > > I assume you are restricting the feature to sendraw so the wallet doesn't get invol
...

💬 maflcko commented on pull request "test: add missing tests for Assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29428#issuecomment-2072998404)
I was asking, because you said you'd update the branch in February: https://github.com/bitcoin/bitcoin/pull/29428#issuecomment-1965145449

💬 theStack commented on pull request "refactor: Rename `subprocess.hpp` to follow our header name conventions":
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2072999520)
Concept ACK