💬 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)

💬 sipa commented on pull request "miniscript: make operator""_mst consteval in C++20 mode":
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-1850268603)
Rebased.

💬 douglaz commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1850275514)
> > Concept ACK.
> > Makes sense to consider the way inserting data as a trick and abuse of the taproot script.
>
> Since the obfuscated data in the script are neither verified nor enforced by the consensus, and these data are to any extent eventually rely on off-chain indexers to proceed, a more elegant way could be to move all data out of the chain for the off-chain indexers to store and deal with, while leaving only a hash on chain which fits well into the design intent with minimal on-ch
...

💬 glozow commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#issuecomment-1850287598)
Closing this as superseded by #28948 and #28984 (v3 and package RBF PRs respectively). Note that the new package RBF has a slightly different approach than the one implemented here, permitting txns based on topology instead of v3-only. The discussion on this PR may still be helpful for context, but the volume of comments makes it pretty hard to do any review here.

✅ glozow closed a pull request: "policy: nVersion=3 and Package RBF"
(https://github.com/bitcoin/bitcoin/pull/25038)

💬 maflcko commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1850288238)
Maybe keep in draft, for as long as it is not reproducible?

💬 maflcko commented on issue "bumpfee doc about incrementalfee is incorrect":
(https://github.com/bitcoin/bitcoin/issues/29053#issuecomment-1850299097)
Both need to be satisfied, whichever is higher will then take precedence, no?

🚀 fanquake merged a pull request: "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction"
(https://github.com/bitcoin/bitcoin/pull/25273)