💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613854692)
used `exact_target` as proposed
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613854692)
used `exact_target` as proposed
💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613782680)
Good question.
It has been a while since I made this change, but if I remember correctly, my fixing the duplicate meaning of `change_cost` broke this bench test due to the following:
Before my change, we would set `change_cost` to `0` as a signal that a transaction does not require a change output. This would happen when a coin selection algorithm found a changeless solution and after creating the recipient outputs there would not be sufficient funds to make a viable change output. However
...
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613782680)
Good question.
It has been a while since I made this change, but if I remember correctly, my fixing the duplicate meaning of `change_cost` broke this bench test due to the following:
Before my change, we would set `change_cost` to `0` as a signal that a transaction does not require a change output. This would happen when a coin selection algorithm found a changeless solution and after creating the recipient outputs there would not be sufficient funds to make a viable change output. However
...
💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613679062)
Adopted the proposed change, but those lines get removed in the next commit anyway.
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613679062)
Adopted the proposed change, but those lines get removed in the next commit anyway.
💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613840073)
Yes! Thanks, I adjusted the comment.
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613840073)
Yes! Thanks, I adjusted the comment.
💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613876620)
I introduced an `exact_target` variable above this test block and used it everywhere it applies
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613876620)
I introduced an `exact_target` variable above this test block and used it everywhere it applies
💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613825421)
Thanks, the "must be 0 if there is no change" is indeed outdated. I removed it.
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613825421)
Thanks, the "must be 0 if there is no change" is indeed outdated. I removed it.
💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613862525)
Added a calculation comment to illustrate the situation:
```code
// = (2 * fee) - (2 * (fee + large_fee_diff)) + change_cost
// = (2 * 100) - (2 * (100 + 90)) + 125
// = 200 - 380 + 125 = -55
```
and asserted that my calculation is correct.
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613862525)
Added a calculation comment to illustrate the situation:
```code
// = (2 * fee) - (2 * (fee + large_fee_diff)) + change_cost
// = (2 * 100) - (2 * (100 + 90)) + 125
// = 200 - 380 + 125 = -55
```
and asserted that my calculation is correct.
💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613850265)
Pulled `exact_target` out as another variable
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613850265)
Pulled `exact_target` out as another variable
💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#issuecomment-2130175396)
> I'm not sure whether [44db79b](https://github.com/bitcoin/bitcoin/commit/44db79b0c48d6b1a26dba6ab01c2a296d610c06b) is useful since it's fully replaced by the following commit.
Ah oops, yeah I guess the first commit was a vestige of my first attempt at this. I squashed it into the second commit.
Diff of before reviews to latest: https://github.com/bitcoin/bitcoin/compare/e95b9159380f2de7f9a6e7a202cc171ad285ee6c..2c18b86
(https://github.com/bitcoin/bitcoin/pull/28366#issuecomment-2130175396)
> I'm not sure whether [44db79b](https://github.com/bitcoin/bitcoin/commit/44db79b0c48d6b1a26dba6ab01c2a296d610c06b) is useful since it's fully replaced by the following commit.
Ah oops, yeah I guess the first commit was a vestige of my first attempt at this. I squashed it into the second commit.
Diff of before reviews to latest: https://github.com/bitcoin/bitcoin/compare/e95b9159380f2de7f9a6e7a202cc171ad285ee6c..2c18b86
💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2130187888)
@instagibbs I've expanded the explanation. Happy to elaborate more if things are unclear.
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2130187888)
@instagibbs I've expanded the explanation. Happy to elaborate more if things are unclear.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1613903504)
The chrono timepoint/duration stuff kind of confuses me. Would this be the idea? (assuming `timeout` is per retry). `sock.Wait` takes milliseconds while the steady clock returns microseconds, so the cast seems unavoidable.
```diff
}
// Wait for response(s) until we get a valid response, a network error, or time out.
- while (true) {
+ auto cur_time = std::chrono::time_point_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now());
+ auto
...
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1613903504)
The chrono timepoint/duration stuff kind of confuses me. Would this be the idea? (assuming `timeout` is per retry). `sock.Wait` takes milliseconds while the steady clock returns microseconds, so the cast seems unavoidable.
```diff
}
// Wait for response(s) until we get a valid response, a network error, or time out.
- while (true) {
+ auto cur_time = std::chrono::time_point_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now());
+ auto
...
💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613915882)
Could do (i think 3?) memcmp here if `is_trivially_copyable_v`, but maybe it's not worth the complexity of differing offsets.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613915882)
Could do (i think 3?) memcmp here if `is_trivially_copyable_v`, but maybe it's not worth the complexity of differing offsets.
💬 epiccurious commented on pull request "doc: Change GiB to GB in release-process.md":
(https://github.com/bitcoin/bitcoin/pull/30171#issuecomment-2130214689)
> Should the value we display depend on platform then?
This makes sense to me for the UI, since the average user will be able to compare values to their local environments in the macOS Files and Windows Explorer applications.
(https://github.com/bitcoin/bitcoin/pull/30171#issuecomment-2130214689)
> Should the value we display depend on platform then?
This makes sense to me for the UI, since the average user will be able to compare values to their local environments in the macOS Files and Windows Explorer applications.
💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613922119)
Trivially copyable does not imply a trivial `operator==`, I think.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613922119)
Trivially copyable does not imply a trivial `operator==`, I think.
💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613925002)
"Objects of trivially-copyable types that are not potentially-overlapping subobjects are the only C++ objects that may be safely copied with [std::memcpy](https://en.cppreference.com/w/cpp/string/byte/memcpy) or serialized to/from binary files with [std::ofstream::write()](https://en.cppreference.com/w/cpp/io/basic_ostream/write) / [std::ifstream::read()](https://en.cppreference.com/w/cpp/io/basic_istream/read)."
I assume anything that can be memcpy'd can be memcmp'd.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613925002)
"Objects of trivially-copyable types that are not potentially-overlapping subobjects are the only C++ objects that may be safely copied with [std::memcpy](https://en.cppreference.com/w/cpp/string/byte/memcpy) or serialized to/from binary files with [std::ofstream::write()](https://en.cppreference.com/w/cpp/io/basic_ostream/write) / [std::ifstream::read()](https://en.cppreference.com/w/cpp/io/basic_istream/read)."
I assume anything that can be memcpy'd can be memcmp'd.
💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613941027)
I certainly see why it's reasonable why trivial types would have a trivial comparison operator, but there is no guarantee for that. I think you can have a trivially-constructible type with an complex `operator==` (for such a type you'd expect that something that was memcpy'd you end up with a result that satisfies ==, but I don't think that's required, and it also doesn't work the other way around: objects could satisfy == without being bitwise identical).
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613941027)
I certainly see why it's reasonable why trivial types would have a trivial comparison operator, but there is no guarantee for that. I think you can have a trivially-constructible type with an complex `operator==` (for such a type you'd expect that something that was memcpy'd you end up with a result that satisfies ==, but I don't think that's required, and it also doesn't work the other way around: objects could satisfy == without being bitwise identical).
💬 TheCharlatan commented on pull request "fuzz: Handle missing BDBRO errors":
(https://github.com/bitcoin/bitcoin/pull/30172#discussion_r1613938921)
Is this still used?
(https://github.com/bitcoin/bitcoin/pull/30172#discussion_r1613938921)
Is this still used?
💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613943540)
Ok, yeah, agreed.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613943540)
Ok, yeah, agreed.
💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1614026697)
I prefer the documentation in https://github.com/jonatack/bitcoin/commit/610397665795b702e380f879db43a000ed81aad3 because it makes clear that for the CLI a different method is needed (`-rpcwallet`).
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1614026697)
I prefer the documentation in https://github.com/jonatack/bitcoin/commit/610397665795b702e380f879db43a000ed81aad3 because it makes clear that for the CLI a different method is needed (`-rpcwallet`).
💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1614029973)
Just adding for posterity because I went down a rabbit hole for this:
It seems clang's `__is_trivially_equality_comparable` builtin would do what we want here:
> Returns true if comparing two objects of the provided type is known to be equivalent to comparing their object representations. Note that types containing padding bytes are never trivially equality comparable.
Not that it's worth using here.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1614029973)
Just adding for posterity because I went down a rabbit hole for this:
It seems clang's `__is_trivially_equality_comparable` builtin would do what we want here:
> Returns true if comparing two objects of the provided type is known to be equivalent to comparing their object representations. Note that types containing padding bytes are never trivially equality comparable.
Not that it's worth using here.