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

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422654904)
Removal of things below minimum relay feerate shouldn't impact the dynamic mempool minimum

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422657588)
Sorry I'm having trouble parsing this suggestion, are you saying we should call `TrimToSize` whenever a transaction is prioritised?

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422670830)
No what I am suggesting is to remove the transaction from mempool when it was deprioritized and its fee reduced to <= 0, not call `TrimToSize`

💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422675622)
"This method can be removed after switching to C++20" was written by me and I think it no longer makes sense.

> But in C++20 it is: std::u8string u8string() const and it is confusing to have a mismatch.

I think this is fine. `fs::path` is not an exact API-copy of `std::filesystem::path`. Some methods are different, or deleted, so this mismatch should be fine.

If a u8string was returned here, UniValue would have to be updated to take it as well, in which case the conversion would need to
...

🚀 fanquake merged a pull request: "build: Enable -Wunreachable-code"
(https://github.com/bitcoin/bitcoin/pull/28999)

💬 maflcko commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1422682244)
nit in 104b3d39f416d20ab496d753afbe7e4d31902065:

This only allows the `version` option for `create*` calls. I think it would be better to allow the option for any MiniWallet method. In python this can be achieved by passing keyword-args down a call chain. See https://github.com/bitcoin/bitcoin/pull/28972. Feel free to cherry-pick that here (replacing this commit), or review it, or ignore it.