🚀 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)
💬 RandyMcMillan commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#issuecomment-1529782191)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/26415#issuecomment-1529782191)
Concept ACK
💬 RandyMcMillan commented on issue "ci: failure in Docker build step":
(https://github.com/bitcoin/bitcoin/issues/27492#issuecomment-1529795351)
Have you tried adding --no-cache flag to the docker build command?
(https://github.com/bitcoin/bitcoin/issues/27492#issuecomment-1529795351)
Have you tried adding --no-cache flag to the docker build command?
💬 instagibbs commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1181639284)
I don't think this is necessary. `get_utxos` already filters for mature coinbases, and you're making 60 of those above.
(https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1181639284)
I don't think this is necessary. `get_utxos` already filters for mature coinbases, and you're making 60 of those above.
📝 brunoerg opened a pull request: "fuzz: addrman, add coverage for `network` field in `Select()`"
(https://github.com/bitcoin/bitcoin/pull/27549)
This PR adds fuzz coverage for `network` field in `Select()`, there was only a call to `Select` without passing a network.
https://marcofalke.github.io/b-c-cov/fuzz.coverage/src/addrman.cpp.gcov.html
(https://github.com/bitcoin/bitcoin/pull/27549)
This PR adds fuzz coverage for `network` field in `Select()`, there was only a call to `Select` without passing a network.
https://marcofalke.github.io/b-c-cov/fuzz.coverage/src/addrman.cpp.gcov.html