💬 aureleoules commented on pull request "ci: Add test coverage job":
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1529678391)
A backwards compatibility test seems to fail because of BDB, I will fix it if this PR gets Concept ACKed.
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1529678391)
A backwards compatibility test seems to fail because of BDB, I will fix it if this PR gets Concept ACKed.
💬 achow101 commented on pull request "rpc: simplify scan blocks":
(https://github.com/bitcoin/bitcoin/pull/26780#issuecomment-1529689713)
ACK b922f6b5262884f42d7483f1e9af35650bdb53a7
(https://github.com/bitcoin/bitcoin/pull/26780#issuecomment-1529689713)
ACK b922f6b5262884f42d7483f1e9af35650bdb53a7
💬 josibake commented on pull request "Add fee_est tool for debugging fee estimation code":
(https://github.com/bitcoin/bitcoin/pull/10443#issuecomment-1529690116)
Haven't dug into the code, but I'm leaning toward Concept ACK, Approach NACK
As someone who is planning to do fee estimation research in the near future, I don't think I'd use something like this the way it's currently written. I would much prefer something that logs Bitcoin Core's live _fee rate_ estimations, or some option that would allow me to tell Bitcoin Core it's at a certain chain height, feed it some mempool data, and then ask it for fee estimations.
I also agree that the tool for
...
(https://github.com/bitcoin/bitcoin/pull/10443#issuecomment-1529690116)
Haven't dug into the code, but I'm leaning toward Concept ACK, Approach NACK
As someone who is planning to do fee estimation research in the near future, I don't think I'd use something like this the way it's currently written. I would much prefer something that logs Bitcoin Core's live _fee rate_ estimations, or some option that would allow me to tell Bitcoin Core it's at a certain chain height, feed it some mempool data, and then ask it for fee estimations.
I also agree that the tool for
...
🚀 achow101 merged a pull request: "rpc: simplify scan blocks"
(https://github.com/bitcoin/bitcoin/pull/26780)
(https://github.com/bitcoin/bitcoin/pull/26780)
💬 josibake commented on pull request "wallet: Replace `GetBalance()` logic with `AvailableCoins()`":
(https://github.com/bitcoin/bitcoin/pull/26756#issuecomment-1529697719)
I'd also prefer we stopped using `AvailableCoins` as a general wallet traversal tool in favor of something like https://github.com/bitcoin/bitcoin/pull/27286
A lot of bugs/strangeness in the wallet seems to come from the fact that we always need to iterate over a bunch of transactions to learn the state of our wallet. If we did track wallet state in one place, we could have well-scoped individual functions for specific tasks.
(https://github.com/bitcoin/bitcoin/pull/26756#issuecomment-1529697719)
I'd also prefer we stopped using `AvailableCoins` as a general wallet traversal tool in favor of something like https://github.com/bitcoin/bitcoin/pull/27286
A lot of bugs/strangeness in the wallet seems to come from the fact that we always need to iterate over a bunch of transactions to learn the state of our wallet. If we did track wallet state in one place, we could have well-scoped individual functions for specific tasks.
💬 achow101 commented on pull request "test: Add test for `sendmany` rpc that uses `subtractfeefrom` parameter":
(https://github.com/bitcoin/bitcoin/pull/26733#issuecomment-1529706820)
ACK 057057a2d7e23c2e29cbfd29a5124b3947a33757
(https://github.com/bitcoin/bitcoin/pull/26733#issuecomment-1529706820)
ACK 057057a2d7e23c2e29cbfd29a5124b3947a33757
🚀 fanquake merged a pull request: "test: add coverage for `-bantime`"
(https://github.com/bitcoin/bitcoin/pull/26604)
(https://github.com/bitcoin/bitcoin/pull/26604)
💬 michaelfolkson commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529711107)
@0xB10C: Thanks for this, great analysis.
> I'm generally in favor of slimming down and removing the message (and also the BIP-37 implementation) at some point.
I read that as a longer term Concept ACK for removal but you'd like a deprecation release schedule rather than removing it immediately in say the next major version release. Presumably something like a deprecation warning in the next major version release before actual deprecation in the following major version release could work f
...
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529711107)
@0xB10C: Thanks for this, great analysis.
> I'm generally in favor of slimming down and removing the message (and also the BIP-37 implementation) at some point.
I read that as a longer term Concept ACK for removal but you'd like a deprecation release schedule rather than removing it immediately in say the next major version release. Presumably something like a deprecation warning in the next major version release before actual deprecation in the following major version release could work f
...
🚀 achow101 merged a pull request: "test: Add test for `sendmany` rpc that uses `subtractfeefrom` parameter"
(https://github.com/bitcoin/bitcoin/pull/26733)
(https://github.com/bitcoin/bitcoin/pull/26733)
💬 fanquake commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1529712915)
> You could remove the reference to this PR
Yep, dropped.
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1529712915)
> You could remove the reference to this PR
Yep, dropped.
💬 MarcoFalke commented on pull request "ci: Add test coverage job":
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1529712995)
I think it is good to have coverage reports, but I am not sure if it is good to overlay them in the files tab. It might be too invasive? Maybe just a comment with a link is fine?
For the other commits: It would be good for reviewers to explain and motivate them, and explain why they are fine to do and what their tradeoffs are (especially not deleting the gcda files?)
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1529712995)
I think it is good to have coverage reports, but I am not sure if it is good to overlay them in the files tab. It might be too invasive? Maybe just a comment with a link is fine?
For the other commits: It would be good for reviewers to explain and motivate them, and explain why they are fine to do and what their tradeoffs are (especially not deleting the gcda files?)
🚀 fanquake merged a pull request: "test: Remove modinv python util helper function"
(https://github.com/bitcoin/bitcoin/pull/27538)
(https://github.com/bitcoin/bitcoin/pull/27538)
💬 MarcoFalke commented on pull request "ci: Add test coverage job":
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1529714362)
Also, if the other commits are self-motivated and stand-alone, they can be submitted/reviewed/tested in a separate pull
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1529714362)
Also, if the other commits are self-motivated and stand-alone, they can be submitted/reviewed/tested in a separate pull
🚀 fanquake merged a pull request: "[23.x] Additional backports for 23.x"
(https://github.com/bitcoin/bitcoin/pull/27475)
(https://github.com/bitcoin/bitcoin/pull/27475)
💬 fanquake commented on issue "Proposed Timeline for Legacy Wallet and BDB removal":
(https://github.com/bitcoin/bitcoin/issues/20160#issuecomment-1529718871)
> I think the milestones will need to be edited in OP to be moved back by one release?
Moved all the remaining milestones back by a release.
(https://github.com/bitcoin/bitcoin/issues/20160#issuecomment-1529718871)
> I think the milestones will need to be edited in OP to be moved back by one release?
Moved all the remaining milestones back by a release.
💬 fanquake commented on pull request "lint: stop ignoring LIEF imports":
(https://github.com/bitcoin/bitcoin/pull/27507#issuecomment-1529721582)
> contrib/devtools/security-check.py:246: error: "object" has no attribute "header" [attr-defined]
The code is working as expected. This seems to either be a bug in mypy, or an issue with the LIEF stubs. Am following up.
(https://github.com/bitcoin/bitcoin/pull/27507#issuecomment-1529721582)
> contrib/devtools/security-check.py:246: error: "object" has no attribute "header" [attr-defined]
The code is working as expected. This seems to either be a bug in mypy, or an issue with the LIEF stubs. Am following up.
💬 fanquake commented on issue "build: Explore building with musl as libc":
(https://github.com/bitcoin/bitcoin/issues/18110#issuecomment-1529727292)
I think the intial route we'll take for static builds will be to use glibc.
(https://github.com/bitcoin/bitcoin/issues/18110#issuecomment-1529727292)
I think the intial route we'll take for static builds will be to use glibc.
💬 aureleoules commented on pull request "ci: Add test coverage job":
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1529730669)
> I think it is good to have coverage reports, but I am not sure if it is good to overlay them in the files tab. It might be too invasive? Maybe just a comment with a link is fine?
You can toggle the annotations in the Files tab client-side using the `a` key.
Or we can fully disable the annotations feature with a custom `codecov.yml` config file but since they are togglable I think it's fine.
It would be nice if Drahtbot could link to the Codecov report also.
> For the other commits: I
...
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1529730669)
> I think it is good to have coverage reports, but I am not sure if it is good to overlay them in the files tab. It might be too invasive? Maybe just a comment with a link is fine?
You can toggle the annotations in the Files tab client-side using the `a` key.
Or we can fully disable the annotations feature with a custom `codecov.yml` config file but since they are togglable I think it's fine.
It would be nice if Drahtbot could link to the Codecov report also.
> For the other commits: I
...
💬 willcl-ark commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529767111)
Thanks for your statistics on the usage of this message @0xB10C, they show a compelling amount of use for the BIP35 protocol still.
As David Harding suggested on the mailing list, which I agree with, removing an actively-used feature in absence of an alternative would be undesirable, and therefore will close this PR for now.
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529767111)
Thanks for your statistics on the usage of this message @0xB10C, they show a compelling amount of use for the BIP35 protocol still.
As David Harding suggested on the mailing list, which I agree with, removing an actively-used feature in absence of an alternative would be undesirable, and therefore will close this PR for now.
✅ willcl-ark closed a pull request: "Remove BIP35 mempool p2p message"
(https://github.com/bitcoin/bitcoin/pull/27426)
(https://github.com/bitcoin/bitcoin/pull/27426)