💬 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.
(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
...
(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.
(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.
(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`.
(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
(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.
(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
...
(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
...
(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
(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
(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
...
(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.
(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.
💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422807954)
> 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?
The underlying data structure is just a container to hold bytes. If the bytes are identical, which they are (in this function), it only matters for readers of the code what the data structure is called.
The important part to getting the right bytes is to call the right function, which is called `u8string`
...
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422807954)
> 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?
The underlying data structure is just a container to hold bytes. If the bytes are identical, which they are (in this function), it only matters for readers of the code what the data structure is called.
The important part to getting the right bytes is to call the right function, which is called `u8string`
...
💬 Retropex commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1850485577)
Well, since people have come to protect their scams business and Core refuses to give the tool to the nodes runners to guard against it, know that @luke-jr maintains a fork of bitcoin core that has already integrated this patch.
Here is the link for [Bitcoin Knot](https://github.com/bitcoinknots/bitcoin).
If you want to stay on bitcoin core, applying a simple [patch](https://gist.github.com/luke-jr/4c022839584020444915c84bdd825831) is enough to get rid of inscription in your mempool.
If
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1850485577)
Well, since people have come to protect their scams business and Core refuses to give the tool to the nodes runners to guard against it, know that @luke-jr maintains a fork of bitcoin core that has already integrated this patch.
Here is the link for [Bitcoin Knot](https://github.com/bitcoinknots/bitcoin).
If you want to stay on bitcoin core, applying a simple [patch](https://gist.github.com/luke-jr/4c022839584020444915c84bdd825831) is enough to get rid of inscription in your mempool.
If
...
💬 martinus commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-1850500567)
> I am wondering if this is worth it at this point, since we only fuzz with llvm based compilers.
I don't know how stable the ordering is for llvm. If it stays like that it's not a big deal I'd say, but it might change in future versions or with optimization levels?
As for stability with other compilers, in a different project I'm working on we run the fuzzing targets with the minimized corpus as unit tests, and this is done in the CI on all platforms. If that would make sense for bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-1850500567)
> I am wondering if this is worth it at this point, since we only fuzz with llvm based compilers.
I don't know how stable the ordering is for llvm. If it stays like that it's not a big deal I'd say, but it might change in future versions or with optimization levels?
As for stability with other compilers, in a different project I'm working on we run the fuzzing targets with the minimized corpus as unit tests, and this is done in the CI on all platforms. If that would make sense for bitcoin
...
💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422828473)
> 2.2. C++20: `char8_t` from which we convert it explicitly to `char`. Is that ok? Why C++20 has `std::u8string` if we can store UTF-8 strings in `std::string`?
Yes, conversion should be fine. See https://stackoverflow.com/a/57453713
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422828473)
> 2.2. C++20: `char8_t` from which we convert it explicitly to `char`. Is that ok? Why C++20 has `std::u8string` if we can store UTF-8 strings in `std::string`?
Yes, conversion should be fine. See https://stackoverflow.com/a/57453713
💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422829381)
> What would be an example in C++20 where it is broken by calling `string()` (1.) and ok by converting `wchar_t`->`char8_t`->`char` (2.2)?
This has nothing to do with C++20. If you call the wrong function `string()` vs `u8string()`, you will also get the wrong result with C++17.
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422829381)
> What would be an example in C++20 where it is broken by calling `string()` (1.) and ok by converting `wchar_t`->`char8_t`->`char` (2.2)?
This has nothing to do with C++20. If you call the wrong function `string()` vs `u8string()`, you will also get the wrong result with C++17.
💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1422836196)
While I like the idea, I don't think it will work as you expect. We are passing `CFeeRate(0)` to `add_coin()`, which generates coins with fee=0, which at the "spending now vs spending in the future" calculation point means a negative waste (because of `waste = fee_to_spend_now - fee_to_spend_in_the_future` --> `waste = 0 - something`). And this negative number increases with the number of selected coins. So, at least in a first glance, this means that the process will always prefer the solution
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1422836196)
While I like the idea, I don't think it will work as you expect. We are passing `CFeeRate(0)` to `add_coin()`, which generates coins with fee=0, which at the "spending now vs spending in the future" calculation point means a negative waste (because of `waste = fee_to_spend_now - fee_to_spend_in_the_future` --> `waste = 0 - something`). And this negative number increases with the number of selected coins. So, at least in a first glance, this means that the process will always prefer the solution
...
💬 maflcko commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1850520824)
> we could wrap the macros in a function
I don't think this works, because it will replace `__FILE__, __LINE__, __func__` with a meaningless string literal constant.
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1850520824)
> we could wrap the macros in a function
I don't think this works, because it will replace `__FILE__, __LINE__, __func__` with a meaningless string literal constant.