Bitcoin Core Github
44 subscribers
121K links
Download Telegram
👋 han0147's pull request is ready for review: "Create pk"
(https://github.com/bitcoin/bitcoin/pull/27187)
fanquake closed a pull request: "Patch 1"
(https://github.com/bitcoin/bitcoin/pull/27186)
📝 fanquake locked a pull request: "Patch 1"
(https://github.com/bitcoin/bitcoin/pull/27186)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...
fanquake closed a pull request: "Create pk"
(https://github.com/bitcoin/bitcoin/pull/27187)
📝 fanquake locked a pull request: "Create pk"
(https://github.com/bitcoin/bitcoin/pull/27187)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...
📝 fanquake locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/27185)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1451488906)
Code review ACK 0bb90cabc8e2c6bbbc3c52ec1170e725435b625d modulo tests.

I believe this PR is a good improvement over status quo.

The biggest worry I have now it somewhat ambiguous terminology:
- `mixed` groups could mean mixed type or mixed effective value
- `all` groups not always refers to actually all groups, but filtered by `CoinEligibiltyFilter`
- two structs `Groups` vs `OutputGroups`

I don't have better suggestion as of now, so it's not blocking. I'll think more and come back
...
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1121297405)
nit: While we are at it, what do you think about passing `wallet.chain()` instead of whole wallet object?
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1121298272)
Nit: this is redundant, we shouldn't have non-spendable coins here at all. They has to be filtered in `AvailableCoins`
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1121303672)
nit: I'd prefer just having two methods for positive only and for all groups. For me this would be cleaner code instead of controlling the flow with bool params.
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1121328934)
The comment is wrong
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1122739758)
nit: it'd be great if we can disambiguate the term `mixed`. Now it applies to both a) mixed type groups b) mixed effects value group
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1121336522)
Not sure why this is needed
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1122748374)
nit: `All` groups is somewhat misleading here, because these are actually filtered groups, but for all output types. Unfortunately, I don't have better suggestion yet.
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1122749611)
nit: Judging from the name it's not clear what's the difference between `Groups` and `OutputGroups`
💬 MarcoFalke commented on pull request "build: use _FORTIFY_SOURCE=3":
(https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1451572322)
This triggered https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56529 ?
💬 MarcoFalke commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1451587856)
ci didn't run?
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1122931688)
Mm thanks, I agree. Was able to reproduce and trace the issue. @Xekyo see previous suggestion to fix.
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1122928753)
This fixes the crash from https://github.com/bitcoin/bitcoin/pull/27021/files#r1121611301

```suggestion
auto it = available_coins.begin();
mtx.vin.push_back(CTxIn(*it, CScript()));
available_coins.erase(available_coins.begin(), it);
```
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1122932387)
Also remember to delete the "All outputs available to spend" comment