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

💬 TheCharlatan commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-1850461907)
Concept ACK

👍 dergoegge approved a pull request: "refactor: rpc: Pass CBlockIndex by reference instead of pointer"
(https://github.com/bitcoin/bitcoin/pull/29021#pullrequestreview-1775657828)
Code review ACK fa5989d514d246e56977c528b2dd2abe6dc8efcc

💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422802518)
I need to understand this better. What I grasp now is that in C++17 it was ok to store UTF-8 strings in `std::string` and in C++20 it is not and one has to use `std::u8string` instead?

This smells because if callers need `std::string` why do they call `u8string()` and not `string()`!? Is it because it was ok to do that in C++17 because in C++17 `std::filesystem::path::string()` would return `std::string` itself and on top of this it was needed because of some complication on Windows?

On Wi
...

👍 vasild approved a pull request: "refactor: Remove pre-C++20 code, fs::path cleanup"
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1775665416)
ACK 856c88776f8486446602476a1c9e133ac0cff510

I realized that this discussion https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422242907 is not a blocker for this PR because it merely removes a comment and changes `const auto` to `const u8string` which is noop, just a style thing.