💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382115704)
Done

💬 sr-gi commented on pull request "net: improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#issuecomment-1792979468)
> tACK [0420f9](https://github.com/bitcoin/bitcoin/commit/0420f99f429ce2382057e101859067f40de47be0)

Thanks for reviewing! Would you mind sharing what did you test and how?

@vasild would you mind re-acking this when you have some time?

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382119240)
Thank you @glozow
Now using two structs `RemovedMempoolTransactionInfo` for this callback and `NewMempoolTransactionInfo` for `TransactionAddedToMempool callback.

Since they have similar fields, I created a base struct with all the similar fields so that they can both inherit 79803d3912747eb96153357f1cc33c3f8cf21eb6 and a469d937dbbadedeb58e0d5d1c44ed7881a7ebb9

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382119456)
Fixed

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382119621)
Added

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382119946)
Thank you, this is removed in latest push

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382120053)
Fixed

🤔 mzumsande reviewed a pull request: "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1713405725)
> Hoping to get some more feedback from @mzumsande and @naumenkogs to proceed, or abandon

Personally, I'm not sure if the use case (running a node with `-maxconnections < 40` and at the same time wanting certain inbound peers, or wanting a really high number of whitelisted inbounds peers at the same time) is common enough to introduce an extra permission just for that. In more typical cases, `noban` should be sufficient.
But if others think it is I'm not against it.

A related use case I c
...

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382126244)
Thank you

This is fixed.
1. a469d937dbbadedeb58e0d5d1c44ed7881a7ebb9 introduces `NewMempoolTransactionInfo` with all the necessary fields and updates `TransactionAddedToMempool` callback parameter.
2. ac82f58e45421aa5aec1d3e6e992d1c369472a10 updates the fee estimator from `CValidationInterface` notifications and manage delegating `validForFeeEstimation` to `CBlockPolicyEstimator::processTransaction` together the cleanups.

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382126431)
Done

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382126780)
Fixed

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#issuecomment-1793005174)
Thanks for your review @glozow and @TheCharlatan
Forced pushed from 6d676c69195dc9032c4558987fb88f4c4b71092c to 09436b21e84cc6cd979fe4942ba1c415c4bc91be
[Compare changes](https://github.com/bitcoin/bitcoin/compare/6d676c69195dc9032c4558987fb88f4c4b71092c..09436b21e84cc6cd979fe4942ba1c415c4bc91be)

- Rework the commits in the order of this [review comment](https://github.com/bitcoin/bitcoin/pull/28368#pullrequestreview-1704427495) which is more cleaner and less verbose.
- Addressed @glozow
...

🤔 instagibbs reviewed a pull request: "validation: return more helpful results for reconsiderable fee failures and skipped transactions"
(https://github.com/bitcoin/bitcoin/pull/28785#pullrequestreview-1713411958)
LGTM with a few suggestions https://github.com/bitcoin/bitcoin/pull/28785/commits/6b0dc669886c07a81fdfbf2d1a71a77ca3bac1c3

💬 instagibbs commented on pull request "validation: return more helpful results for reconsiderable fee failures and skipped transactions":
(https://github.com/bitcoin/bitcoin/pull/28785#discussion_r1382139875)
`Wtxid`? :point_right: :point_left:

💬 instagibbs commented on pull request "validation: return more helpful results for reconsiderable fee failures and skipped transactions":
(https://github.com/bitcoin/bitcoin/pull/28785#discussion_r1382138716)
```suggestion
// Failed for fee reasons. Provide the effective feerate and which txn was included.
```

💬 instagibbs commented on pull request "validation: return more helpful results for reconsiderable fee failures and skipped transactions":
(https://github.com/bitcoin/bitcoin/pull/28785#discussion_r1382126634)
Might make sense to add a comment to the effect that we don't want to reject fetching this same tx in a future package

💬 instagibbs commented on pull request "Fuzz: Check individual and package transaction invariants":
(https://github.com/bitcoin/bitcoin/pull/28764#discussion_r1382150706)
missing some `return` statements in this function

📝 instagibbs opened a pull request: "test: bugfix CheckPackageMempoolAcceptResult return all error strings"
(https://github.com/bitcoin/bitcoin/pull/28788)
Noticed on follow-up testing work https://github.com/bitcoin/bitcoin/pull/28764/files#r1382150706

💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1382141691)
In 770a73c66a48434fa48f594709dbbf5831dc1d5d "wallet: track mempool conflicts"

nit: `snake_case` for member variables.

Additionally, this is tracking these conflicts at the `CWallet` level. However I wonder if it might be better to track it on a transaction level, i.e. have `CWalletTx` have the set of txids.

💬 ariard commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1793036667)
I think it’s good to communicate on the mailing list in which version (27.0 or 28.0) full-rbf might be turn on by default. I advocated such setting in the past as early as 0.24, though at the time some 0-conf transactions applications and services were migrating their stuff in consequence.