💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581147730)
Much cleaner now

💬 sybenx commented on pull request "fix block subsidy at 3.25 BTC":
(https://github.com/bitcoin/bitcoin/pull/29778#issuecomment-2079554427)
3.125 is way too high. Once the risk becomes apparent, then a number would be obvious, if necessary

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581157473)
Why caching this in `m_recent_rejects_reconsiderable` instead of `m_recent_rejects` if we are not going to reconsider them?

💬 brunoerg commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#issuecomment-2079566994)
Concept ACK

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1579748212)
nit: `tx_parent_3` is the same transaction `tx_parent_1` maybe just use `tx_parent_1` as parent of `tx_child_3`

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580896456)
I think this might be a better name
```suggestion
def test_package_rbf_with_conflicting_packages(self):
```

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580798337)
You moved `fill_mempool` to util in #29735

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1579508562)
Question: here you mean the package RBF rules in OP not BIP125 right?

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580946063)
nit:
```suggestion
def test_package_rbf_with_numerous_ancestors(self):
```

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580918022)
The parent transaction will be detected as `inmempool` as such the child just get added to the mempool if it passes normal transaction consensus and policy rules, the package does not have any conflict and hence package RBF rules are not checked.

This test also passes when I removed package RBF commits
```suggestion
def test_submitpackage_with_a_mempool_parent(self):
self.log.info("Test that submitpackage works when parent transactions was already submitted")
```

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580973594)
```suggestion
# Package 2 feerate is below the feerate of directly conflicted parent, even though
```

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1579749486)
`package1` and `package3` has the same parent

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580970962)
nit:
```suggestion
def test_package_rbf_with_wrong_pkg_size(self):
```

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1581088045)
nice this will be helpful for collecting data, previously we only know that a transaction is replaced now it indicates which tx replaced it!

E.g a transaction evicting two transaction package evicting another two
```terminal
2024-04-26T14:36:53.889903Z [httpworker.3] [validation.cpp:1282] [Finalize] [mempool] replacing mempool tx 1ae8118ceb71249b2af793f671a1293a4c999e89f5091f52c42a3cf1e08c6840 (wtxid=97fc8654dcbd025b4165c56179d124ee4adf2d65acf8a66f8f5464dab7d07089, fees=10000, vsize=104). N
...

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1581185454)
Ok, agreed

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2079604399)
> ACK [be2c551](https://github.com/bitcoin/bitcoin/commit/be2c5510521081119f62ef3b29f76bb9097d0f34) Re-reviewed code, and attempted to break tests by making `ConsiderEviction` misbehave.
>
> It is worth noting that these tests will not catch if outbound eviction logic gets triggered too early. e.g.:
>
> ```c++
> /** How long to wait for a peer to respond to a getheaders request */
> static constexpr auto HEADERS_RESPONSE_TIME{0s};
> // [...]
> /** Timeout for (unprotected) outbound pee
...

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2079606752)
Updated to address https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1580221364

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2079616145)
re: https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2079388456

> > or the [CompleteChainstateInitialization](https://github.com/ryanofsky/bitcoin/blob/55d7de92bbbe035c1833c89f885af14e5b243932/src/node/chainstate.cpp#L38-L179) function from #25665
>
> I don't think this is good usage of `Update()`, since there is no chaining happening here. I would find list initialization more readable and less error-prone:
>
> git diff on 1376583

The diff you suggested is actually the way
...

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2079619223)
I'm not a Rust guru, but if someone can write a quick script to make `rust-silentpayments` spit out the tweaks per block, I can compare it with our index.

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581204910)
Ok, I've pushed a commit. Let me know what you think.