💬 jonatack commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1881690072)
> forcing usage of a single `Log` function
That doesn't make sense if the goal, like the title of this PR, is to simplify the API. One macro is simpler, and less code and test coverage, than five.
I plan to review here soon. My work on severity-based logging in https://github.com/bitcoin/bitcoin/pull/25203 was set aside to first fix current bugs, and add missing tests and documentation in https://github.com/bitcoin/bitcoin/pull/27231 that has been open for ten months.
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1881690072)
> forcing usage of a single `Log` function
That doesn't make sense if the goal, like the title of this PR, is to simplify the API. One macro is simpler, and less code and test coverage, than five.
I plan to review here soon. My work on severity-based logging in https://github.com/bitcoin/bitcoin/pull/25203 was set aside to first fix current bugs, and add missing tests and documentation in https://github.com/bitcoin/bitcoin/pull/27231 that has been open for ten months.
💬 ajtowns commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445230410)
Until we can use [std::source_location](https://en.cppreference.com/w/cpp/utility/source_location) (see https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1852024695) we need to use macros in order to pass `__func__`, `__FILE__` and `__LINE__` information through.
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445230410)
Until we can use [std::source_location](https://en.cppreference.com/w/cpp/utility/source_location) (see https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1852024695) we need to use macros in order to pass `__func__`, `__FILE__` and `__LINE__` information through.
💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1881694073)
I'm going to open an alternate pull that does what you suggest and see what reviewers (if any) prefer.
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1881694073)
I'm going to open an alternate pull that does what you suggest and see what reviewers (if any) prefer.
💬 reardencode commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1881695275)
Force pushed to fix clean commits.
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1881695275)
Force pushed to fix clean commits.
🤔 murchandamus reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1800970305)
Addressed @sipa’s comments, and completely restructured the PR. Added two more tests, and counting of the number of attempts used in CoinGrinder to measure the optimizations.
Ready for review
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1800970305)
Addressed @sipa’s comments, and completely restructured the PR. Added two more tests, and counting of the number of attempts used in CoinGrinder to measure the optimizations.
Ready for review
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445276896)
Thanks, I replaced all the `std::vector::at` with `std::vector::operator[]`.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445276896)
Thanks, I replaced all the `std::vector::at` with `std::vector::operator[]`.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1439793432)
I have reduced the description here to the interface and will add the remaining explanations to the corresponding spots of the function.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1439793432)
I have reduced the description here to the interface and will add the remaining explanations to the corresponding spots of the function.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1439882229)
I have completely rewritten my description. I hope it’s clearer now.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1439882229)
I have completely rewritten my description. I hope it’s clearer now.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445277299)
I’ve completely rewritten CoinGrinder to implement this suggestion.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1445277299)
I’ve completely rewritten CoinGrinder to implement this suggestion.
👋 murchandamus's pull request is ready for review: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877)
(https://github.com/bitcoin/bitcoin/pull/27877)
💬 achow101 commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445244468)
In 4e746e223a016c00abcaece6002fbbf37ff19f73 "[mempool] evict everything paying 0 fees in TrimToSize()"
This seems like it should belong in a separate commit? At least the motivation for this is not entirely clear from the code comments nor the commit message.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445244468)
In 4e746e223a016c00abcaece6002fbbf37ff19f73 "[mempool] evict everything paying 0 fees in TrimToSize()"
This seems like it should belong in a separate commit? At least the motivation for this is not entirely clear from the code comments nor the commit message.
💬 achow101 commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445293662)
In 299680a5c736783fbb03fd448064ed35ef1a42a0 "[policy] add v3 policy rules"
nit: Could use `GetVirtualTransactionSize` instead of calculated the sigops adjusted size manually.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445293662)
In 299680a5c736783fbb03fd448064ed35ef1a42a0 "[policy] add v3 policy rules"
nit: Could use `GetVirtualTransactionSize` instead of calculated the sigops adjusted size manually.
💬 achow101 commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445328322)
In fd55f70257456e804cadd7af3e70f783de373008 "[functional test] v3 transaction submission"
nit: use `self.check_mempool`, here and elsewhere.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445328322)
In fd55f70257456e804cadd7af3e70f783de373008 "[functional test] v3 transaction submission"
nit: use `self.check_mempool`, here and elsewhere.
💬 achow101 commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445273282)
In 299680a5c736783fbb03fd448064ed35ef1a42a0 "[policy] add v3 policy rules"
nit: This could be at the top of the loop as well instead of retrieving it for every input.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445273282)
In 299680a5c736783fbb03fd448064ed35ef1a42a0 "[policy] add v3 policy rules"
nit: This could be at the top of the loop as well instead of retrieving it for every input.
💬 achow101 commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445305284)
In 6de06e116f0dbe632be0e4babd45d3f59c117171 "[policy/validation] allow v3 transactions with certain restrictions"
This seems unnecessary since if the tx was v3, then we wouldn't be able to reach this error condition.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1445305284)
In 6de06e116f0dbe632be0e4babd45d3f59c117171 "[policy/validation] allow v3 transactions with certain restrictions"
This seems unnecessary since if the tx was v3, then we wouldn't be able to reach this error condition.
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1881840566)
Rebased and big refactor.
Introducing `Sv2Transport`, a subclass of `Transport` very similar to `V2Transport`. For the most part this abstraction works quite well, and should make it easy to review for people familiar with the v2 transport (BIP324) implementation.
There'a few pain points though, perhaps @sipa has thoughts on this?
1. `CNetMessage` and `Sv2NetMsg` can perhaps be united to a single base class, but there are differences: `Sv2NetMsg` does not need `std::string m_type`. This
...
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1881840566)
Rebased and big refactor.
Introducing `Sv2Transport`, a subclass of `Transport` very similar to `V2Transport`. For the most part this abstraction works quite well, and should make it easy to review for people familiar with the v2 transport (BIP324) implementation.
There'a few pain points though, perhaps @sipa has thoughts on this?
1. `CNetMessage` and `Sv2NetMsg` can perhaps be united to a single base class, but there are differences: `Sv2NetMsg` does not need `std::string m_type`. This
...
💬 murchandamus commented on pull request "test: Add algo assert to bnb_search_test":
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1881850647)
Thanks @yancyribbens for your report. There are a number of issues with these tests specifically and the `coinselector_tests` suite in general. They recently resurfaced when we were addressing an issue with SFFO and BnB via #28994. In the specific test with `result13`, the `9` is a preselected input and therefore BnB is returning 9+1 instead of 10.
Either way, you are looking at hopefully soon obsoleteded tests. In conjunction with that bugfix I started working on completely rewriting the `co
...
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1881850647)
Thanks @yancyribbens for your report. There are a number of issues with these tests specifically and the `coinselector_tests` suite in general. They recently resurfaced when we were addressing an issue with SFFO and BnB via #28994. In the specific test with `result13`, the `9` is a preselected input and therefore BnB is returning 9+1 instead of 10.
Either way, you are looking at hopefully soon obsoleteded tests. In conjunction with that bugfix I started working on completely rewriting the `co
...
💬 achow101 commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1445381354)
In d8850285d9ebfbff77b6f04f80f1682a15c8783a "refactor: simplify `CreateRecipients`"
Using a `UniValue` in this function seems kinda clunky. I would much rather than the parsing be done by the RPCs that need to do the parsing and this function could just take a `std::vector<std::string> sffo_addrs` or something like that. None of these options seems to exist in other RPCs.
Since all of the different ways that sffo is set by the user, this could do type interpretation to figure it out. If it
...
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1445381354)
In d8850285d9ebfbff77b6f04f80f1682a15c8783a "refactor: simplify `CreateRecipients`"
Using a `UniValue` in this function seems kinda clunky. I would much rather than the parsing be done by the RPCs that need to do the parsing and this function could just take a `std::vector<std::string> sffo_addrs` or something like that. None of these options seems to exist in other RPCs.
Since all of the different ways that sffo is set by the user, this could do type interpretation to figure it out. If it
...
💬 achow101 commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1445376055)
In d8850285d9ebfbff77b6f04f80f1682a15c8783a "refactor: simplify `CreateRecipients`"
Can be simplified with just a normal for loop:
```suggestion
for (size_t i = 0; i < outputs.size(); ++i) {
const auto& [destination, amount] = outputs.at(i);
CRecipient recipient = {destination, amount, subtract_fee_outputs.contains(i)};
recipients.push_back(recipient);
```
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1445376055)
In d8850285d9ebfbff77b6f04f80f1682a15c8783a "refactor: simplify `CreateRecipients`"
Can be simplified with just a normal for loop:
```suggestion
for (size_t i = 0; i < outputs.size(); ++i) {
const auto& [destination, amount] = outputs.at(i);
CRecipient recipient = {destination, amount, subtract_fee_outputs.contains(i)};
recipients.push_back(recipient);
```
💬 achow101 commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1445369299)
In 8ab3da31c56d2cc05c4273ec0833109be106ed2b "refactor: move parsing to new function"
I would that these two variables be declared inside of the loop just to avoid things leaking between iterations. I don't see why they need to be outside of the loop.
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1445369299)
In 8ab3da31c56d2cc05c4273ec0833109be106ed2b "refactor: move parsing to new function"
I would that these two variables be declared inside of the loop just to avoid things leaking between iterations. I don't see why they need to be outside of the loop.
💬 achow101 commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1445383947)
In d8850285d9ebfbff77b6f04f80f1682a15c8783a "refactor: simplify `CreateRecipients`"
Packing the sffo info into a UniValue just to be parsed again seems to be really unergonomic.
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1445383947)
In d8850285d9ebfbff77b6f04f80f1682a15c8783a "refactor: simplify `CreateRecipients`"
Packing the sffo info into a UniValue just to be parsed again seems to be really unergonomic.