📝 theStack opened a pull request: "init: completely remove `-zapwallettxes` (remaining hidden option)"
(https://github.com/bitcoin/bitcoin/pull/28787)
The `-zapwallettxes` functionality has been removed in v0.21.0 (see commit 3340dbadd38f5624642cf0e14dddbe6f83a3863b / PR #19671), with the parameter being kept as hidden option, to inform users via an exit error that `abandontransaction` should be used instead.
As any guides that still suggest to use `-zapwallettxes` would refer to a Bitcoin Core version that is EOL since many years (i.e. <= v0.20.x), it is highly unlikely that the error caused by the option is still relevant for any user, he
...
(https://github.com/bitcoin/bitcoin/pull/28787)
The `-zapwallettxes` functionality has been removed in v0.21.0 (see commit 3340dbadd38f5624642cf0e14dddbe6f83a3863b / PR #19671), with the parameter being kept as hidden option, to inform users via an exit error that `abandontransaction` should be used instead.
As any guides that still suggest to use `-zapwallettxes` would refer to a Bitcoin Core version that is EOL since many years (i.e. <= v0.20.x), it is highly unlikely that the error caused by the option is still relevant for any user, he
...
💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382115534)
Thank you removed
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382115534)
Thank you removed
💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1382115704)
Done
(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?
(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
(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
(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
(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
(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
(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
...
(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.
(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
(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
(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
...
(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
(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:
(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.
```
(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
(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
(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
(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.
(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.