💬 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.
🤔 jonatack reviewed a pull request: "fuzz: set `nMaxOutboundLimit` in connman target"
(https://github.com/bitcoin/bitcoin/pull/29172#pullrequestreview-1810118033)
ACK e5b9ee0221ec8aa238fb5720fcd6faa01b09fe46
(https://github.com/bitcoin/bitcoin/pull/29172#pullrequestreview-1810118033)
ACK e5b9ee0221ec8aa238fb5720fcd6faa01b09fe46
💬 yancyribbens commented on pull request "test: Add algo assert to bnb_search_test":
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1881922860)
> In the specific test with result13, the 9 is a preselected input and therefore BnB is returning 9+1 instead of 10.
Ok. The test reads that it expects [9, 1] as a result AFAICT and since the fee_rate is set to be high (fee_rate > long_term_feerate) it should be 10. I verified that the BnB algo is actually doing the correct thing, although it's good to know there are some fixes coming for the tests.
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1881922860)
> In the specific test with result13, the 9 is a preselected input and therefore BnB is returning 9+1 instead of 10.
Ok. The test reads that it expects [9, 1] as a result AFAICT and since the fee_rate is set to be high (fee_rate > long_term_feerate) it should be 10. I verified that the BnB algo is actually doing the correct thing, although it's good to know there are some fixes coming for the tests.
🤔 furszy reviewed a pull request: "wallet: simplify and batch zap wallet txes process"
(https://github.com/bitcoin/bitcoin/pull/28987#pullrequestreview-1810138430)
CI failure is unrelated. Relates to #29112.
(https://github.com/bitcoin/bitcoin/pull/28987#pullrequestreview-1810138430)
CI failure is unrelated. Relates to #29112.
👍 jamesob approved a pull request: "logging: Simplify API for level based logging"
(https://github.com/bitcoin/bitcoin/pull/28318#pullrequestreview-1810116924)
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
This is a definite improvement in the new-style logging interface.
I built and reviewed each commit locally, and ran `./test/functional/feature_segwit.py --nocleanup` and reviewed the resulting debug.log to sanity check.
Eventually it would be nice to unify the interface for each logging function (
...
(https://github.com/bitcoin/bitcoin/pull/28318#pullrequestreview-1810116924)
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
This is a definite improvement in the new-style logging interface.
I built and reviewed each commit locally, and ran `./test/functional/feature_segwit.py --nocleanup` and reviewed the resulting debug.log to sanity check.
Eventually it would be nice to unify the interface for each logging function (
...
💬 jamesob commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445429093)
https://github.com/bitcoin/bitcoin/pull/28318/commits/f7ce5ac08c669ac763e275bb7c82dcfb2b1b6c33
It's somewhat wonky that e.g. `LogInfo()` doesn't take a category, but e.g. `LogDebug` does, and in later commits we actually remove category information from info-equivalent log statements, which seems backwards to me. Ideally, `LogInfo` would _optionally_ take a category. But maybe this can be done later?
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445429093)
https://github.com/bitcoin/bitcoin/pull/28318/commits/f7ce5ac08c669ac763e275bb7c82dcfb2b1b6c33
It's somewhat wonky that e.g. `LogInfo()` doesn't take a category, but e.g. `LogDebug` does, and in later commits we actually remove category information from info-equivalent log statements, which seems backwards to me. Ideally, `LogInfo` would _optionally_ take a category. But maybe this can be done later?
💬 jamesob commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445407709)
Minor thing, but there's still a lingering reference in `contrib/devtools/bitcoin-tidy/example_logprintf.cpp`. Not sure if that needs to be updated or not.
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445407709)
Minor thing, but there's still a lingering reference in `contrib/devtools/bitcoin-tidy/example_logprintf.cpp`. Not sure if that needs to be updated or not.
💬 stickies-v commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445447535)
Gha forgout about that. Macro it is, thanks.
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1445447535)
Gha forgout about that. Macro it is, thanks.
💬 hernanmarino commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1445448364)
Agree, I'll change it soon
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1445448364)
Agree, I'll change it soon
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1881954408)
Mea culpa. Fixed the linter issue.
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1881954408)
Mea culpa. Fixed the linter issue.