👍 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.
💬 eragmus commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1850530167)
> The shipcoiners have truly arrived, as evidenced by this thread. Thanks, bitcoin core. Unbelievable.
Ad hominem (and certainly not applicable to all commenters), the second lowest level of disagreement. Better to level up & address the actual arguments being made.
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1850530167)
> The shipcoiners have truly arrived, as evidenced by this thread. Thanks, bitcoin core. Unbelievable.
Ad hominem (and certainly not applicable to all commenters), the second lowest level of disagreement. Better to level up & address the actual arguments being made.
💬 theuni commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422852478)
I think I'd prefer to leave this with its comment. Note that we don't use `std::counl_zero` directly, we want kinda the opposite: `(sizeof(x) * 8 )- std::countl_zero(x);`
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422852478)
I think I'd prefer to leave this with its comment. Note that we don't use `std::counl_zero` directly, we want kinda the opposite: `(sizeof(x) * 8 )- std::countl_zero(x);`
👋 josibake's pull request is ready for review: "wallet, rpc: `FundTransaction` refactor"
(https://github.com/bitcoin/bitcoin/pull/28560)
(https://github.com/bitcoin/bitcoin/pull/28560)
💬 maflcko commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422858528)
Can remove this from configure.ac now?
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422858528)
Can remove this from configure.ac now?
💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#issuecomment-1850549633)
For the reviewers of this PR, https://github.com/bitcoin/bitcoin/pull/28560 is a follow-up that further refactors `FundTransaction` by cleaning up the SFFO parsing and removing the SFFO logic from `FundTransaction`. It does this by passing a `CRecipient` vector to `FundTransaction`, which also sets us up to be able to pass silent payment addresses to `FundTransaction` in a later PR.
(https://github.com/bitcoin/bitcoin/pull/25273#issuecomment-1850549633)
For the reviewers of this PR, https://github.com/bitcoin/bitcoin/pull/28560 is a follow-up that further refactors `FundTransaction` by cleaning up the SFFO parsing and removing the SFFO logic from `FundTransaction`. It does this by passing a `CRecipient` vector to `FundTransaction`, which also sets us up to be able to pass silent payment addresses to `FundTransaction` in a later PR.
💬 theuni commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422867877)
I worked on a branch last week that left me very confused: https://github.com/theuni/bitcoin/commits/pr28674-rebase2/
It's the _last_ commit (which I would expect to be a no-op) which causes the slowdown. For whatever reason, the built-in functions like `be64toh` are much faster than my hand-written internal ones which implement _the same code that glibc does_ in its headers! I'm still scratching my head trying to work out what's happening.
But for now, now that we have c++20, I'll go ahe
...
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422867877)
I worked on a branch last week that left me very confused: https://github.com/theuni/bitcoin/commits/pr28674-rebase2/
It's the _last_ commit (which I would expect to be a no-op) which causes the slowdown. For whatever reason, the built-in functions like `be64toh` are much faster than my hand-written internal ones which implement _the same code that glibc does_ in its headers! I'm still scratching my head trying to work out what's happening.
But for now, now that we have c++20, I'll go ahe
...
💬 theuni commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422871347)
Looks like it. Will do in the updated PR.
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422871347)
Looks like it. Will do in the updated PR.
💬 maflcko commented on pull request "[POC][WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1850577489)
Needs rebase
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1850577489)
Needs rebase
💬 maflcko commented on pull request "[POC][WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1850578430)
> WIP: our `prevector` implementation requires modification for `std::ranges::views::reverse` to work. More specifically: it needs to implement the [`bidirectional_range`](https://en.cppreference.com/w/cpp/ranges/bidirectional_range) concept, which currently it seems not to (see `static_assert(std::ranges::bidirectional_range<prevector>)` output below) as we're not satisfying [`range`](https://en.cppreference.com/w/cpp/ranges/range). Once that's done, we can update the last usage of `reverse_ite
...
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1850578430)
> WIP: our `prevector` implementation requires modification for `std::ranges::views::reverse` to work. More specifically: it needs to implement the [`bidirectional_range`](https://en.cppreference.com/w/cpp/ranges/bidirectional_range) concept, which currently it seems not to (see `static_assert(std::ranges::bidirectional_range<prevector>)` output below) as we're not satisfying [`range`](https://en.cppreference.com/w/cpp/ranges/range). Once that's done, we can update the last usage of `reverse_ite
...
💬 Retropex commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1850614834)
If you can't see yourself that the thousands of tokens created with the inscriptions are scams I really can't do anything for you, for the images it's the same, inscribe ten times the same things with a slight declination and then move on to something else when people are no longer interested. This cycle continues indefinitely.
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1850614834)
If you can't see yourself that the thousands of tokens created with the inscriptions are scams I really can't do anything for you, for the images it's the same, inscribe ten times the same things with a slight declination and then move on to something else when people are no longer interested. This cycle continues indefinitely.