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

💬 maflcko commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1850351548)
> > At least on my machine, this does't catch [#28830 (comment)](https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1415690508) though.
>
> `Assert(!error)` in that case is trivially true (error was just set to nullopt, !nullopt is true), so presumably it's getting compiled to a no-op, at which point there is no unreachable code to warn about?

I did some earlier testing, and I think generally any code injected from macros will be ignored by this warning. Adding a no-op verbatim shou
...

💬 maflcko commented on pull request "wallet: tx creation, don't select outputs from txes that are being replaced":
(https://github.com/bitcoin/bitcoin/pull/26732#issuecomment-1850358358)
> > There hasn't been much activity lately and the patch still needs rebase. What is the status here?
> >
> > * Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
>
> It is still relevant. I'm waiting for #27601 outcome to rebase it. This work depends on it.

Is there an outcome? Seems like nothing has happened on the pull since, other than it needing rebase.

💬 Sun0fABeach commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1850391780)
The shipcoiners have truly arrived, as evidenced by this thread. Thanks, bitcoin core. Unbelievable.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1850396923)
I moved `sv2_noise.h` and `sv2_messages.h` to `common` and `template_provider.h` to `node`.

🤔 mzumsande reviewed a pull request: "doc/reduce-traffic: update/clarify max outbound connection count"
(https://github.com/bitcoin/bitcoin/pull/29052#pullrequestreview-1775617090)
Code Review ACK aa4907ae19c797493939dcbf4762cb1666f86546

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1850443467)
(re-)based on #28880.

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1850445505)
re: https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1849208455

Idea to build util and util-core (or util-lite?) libraries is interesting, because then it would be easy to add/remove things from util library depending on whether the kernel needs them, and it would never require renaming anything. I could see this being useful in the future if util library started growing a lot and accumulating more features of over time.

But for now, IMO, just sticking with the current util/commo
...

💬 stickies-v commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1850459273)
I did some testing too. Adding a `std::cout << "Hello\n";` at the end of `MakeMempool` results in a warning:

<details>
<summary>git diff on fa8adbe7c1</summary>

```diff
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index 5a08d0ff44..2369c2ee0d 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@ -126,6 +126,7 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte

// ...and construct a
...