💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422485145)
the other variant you're referring to here https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421614259 should cover that case.

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1850133512)
The BnB search test was passing before as well, because it was expecting that BnB prefer the two input solution over the one input solution at low feerates, and deducting the fee from the target instead of adding the fee to the inputs caused the two input solution to have a better waste score than the one input solution: because the target was reduced the one input solution had a waste that was higher by one input fee than that of the two input solution.

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1850163591)
> > Applied @murchandamus suggestion of adding inputs fee to each UTXO rather than deduct them from the target. The latter one was breaking the equivalence of the input sets.
>
> Wasn't the test still passing before, when you were deducting them from the target? If so, I would suggest we take a closer look at this test in #28985 to ensure it's testing the thing it's supposed to be testing.

Yes, it was passing but not for the right reason.
Deducting the inputs fees from the target was the
...

🤔 ismaelsadeeq reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1774659127)
>There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will continue to use this or something similar afterward.

If IIUC the pinning issues that are fixed by this new v3 rules are
1. Rule 3 Absolute fee pinning.
2. Rule 5 pinning pinning vector.


I looked at the proposal of cluster mempool the v3 proposals but fail to understand how the addition of cluster mempool might revert the pinning vectors solved by this new policy rules specifically of
...

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422166975)
why move this to the if statement?

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422494963)
I agree with @mzumsande https://github.com/bitcoin/bitcoin/pull/28948/commits/bb979f4c76f4f4f4357a08674df833a5cc6dcf82#r1414612161.
I think it will generally be better to remove the transaction when it was deprioritized and its fee reduced to <= 0 to clear the ambiguity?

tx2 is evicted for having 0 fees but its when we are trimming mempool that its evicted, it should not be in the mempool the moment it's deprioritized not at a later time.

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422364536)
```suggestion
#include <algorithm>
```

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422497121)
```cpp
if (ancestors.empty()) {
return std::nullopt;
}
```
should be checked before we check this size?

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422429402)

Is it worth adding a comment saying `Package` does not refer to a child and all of its unconfirmed parents? The `Package` in param can contain unrelated transaction.
untill I saw the test, I had the assumption of otherwise.

🤔 murchandamus requested changes to a pull request: "wallet: skip BnB when SFFO is enabled"
(https://github.com/bitcoin/bitcoin/pull/28994#pullrequestreview-1775245161)
Looks much better. I have another idea how to improve the `tx_creation_bnb_sffo_restriction` test, which I think would make it clearer what is being tested there

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1422530242)
Since there is only a single input, all algorithms that produce a solution would necessarily use the same input set. It was not obvious to me why we expect the BnB solution to be the one returned if it was produced. It seems to me that it is because BnB runs first and we prefer the first equivalent solution if we have multiple.
If that is right that feels like a brittle assumption to test. What if e.g. someone introduced another coin selection algorithm later that is run before BnB? The test wo
...

💬 murchandamus commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1850184563)
Thanks both of you!

@kashifs: If you open a PR against my PR or send me a patch by email, I’d be happy to add your commit with the additional tests to the PR.

💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1850227162)
Changed the approach.

This now still deletes all the adjusted time code and removes it from consensus code but keeps the warning condition for a large median time offset as it is on master. Improvements to the warning can be done in a follow up.

This should now be a very straight forward review, since we don't need to discuss a new warning mechanism.

💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1850227745)
> Also sent a patch to update LLVM in Guix to 17.0.6: https://lists.gnu.org/archive/html/guix-patches/2023-12/msg00226.html.

This landed, so will use LLVM 17.0.6 in Guix now as well.

💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1422593402)
Kept the setting in the new approach

💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1422595852)
This is documented in the current code.
https://github.com/bitcoin/bitcoin/blob/40bc501bf462e1b38679f728336f18f08ee251ca/src/timedata.cpp#L60-L76

💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1422596329)
With the new approach this is kept as is on master

💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1422596929)
This wasn't implemented before either. This can be improved in a follow up.

💬 pinheadmz commented on pull request "Make bitcoin-tx replaceable value optional":
(https://github.com/bitcoin/bitcoin/pull/29022#discussion_r1422615111)
Ok I see maybe then `"If the transaction has no inputs, this option is ignored"`? is that correct?

🚀 fanquake merged a pull request: "fuzz: p2p: Detect peer deadlocks"
(https://github.com/bitcoin/bitcoin/pull/29009)