📝 Sjors opened a pull request: "Mining interface followups"
(https://github.com/bitcoin/bitcoin/pull/30335)
Followups from #30200

- `std::unique_ptr` needs `#include <memory>` (noticed while working on #30332, which has fewer includes than its parent PR that I originally tested with)
- move out argument `state` to the end of `testBlockValidity`, see https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1647987176

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1652627253)
Actually, doing this now since I ran into another (minor) issue: #30335

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1652645443)
Fixed in #30335

💬 Sjors commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1652647962)
Added blank line here for consistency.

💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1652648077)
That does look nicer, implementing your suggestion. Thanks for teaching me `.partition()` and `.startswith()`.

💬 ismaelsadeeq commented on pull request "[test]: prevent `create_self_transfer` failure when target weight is below tx weight":
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1652649739)
updated.

💬 stickies-v commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1652654157)
Great. I didn't mean to take out your example docstrings btw so feel free to add those/similar ones in, it's helpful.

💬 paplorinc commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#discussion_r1652654540)
The [commit](https://github.com/bitcoin/bitcoin/commit/400b15147cf1c4757927935222611e8d3481e739) by @morcos indicates there's still use for the print statement - even though it wasn't used correctly in the tests, I've removed that reference.

💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1652656319)
```python3
def parse_version_string(version_str):
parts = version_str.split('-')
# "<version>" or "<version>-platform"
version_base, _, version_os = version_str.partition('-')
version_rc = ""
if version_os.startswith("rc"): # "<version>-rcN[-platform]"
version_rc, _, version_os = version_os.partition('-')

return version_base, version_rc, version_os
```

Look good to you?

💬 stickies-v commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1652658769)
Actually: would `version`, `rc`, `platform` be better variable names?

💬 stickies-v commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1652661626)
> Look good to you?

No need for the `parts = ` statement I think.

> `# "<version>" or "<version>-platform"`

That seems wrong, given that we may also have an `rc` part still?

💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1652677181)
All of your comments are right! Glad I asked.

Looks better with the new local variable names:

```python3

def parse_version_string(version_str):
# "<version>[-rcN][-platform]"
version_base, _, platform = version_str.partition('-')
rc = ""
if platform.startswith("rc"): # "<version>-rcN[-platform]"
rc, _, platform = platform.partition('-')
# else "<version>" or "<version>-platform"

return version_base, rc, platform
```

I don't love my first comm
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2188786279)
Rebased after #30200 landed! 🎉 Reorganized commits so it's now based on #30332.

💬 maflcko commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#discussion_r1652705500)
See "Lost baseline coverage " in https://corecheck.dev/bitcoin/bitcoin/pulls/30324. So this seems wrong

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2188840483)
I dropped b9fdd0dc75b5b4944dffc700b0391b38465f754a as suggested by @ryanofsky so the PR is focussed on Cirrus.

I updated 026d86427527a6c58425896066386713b9782275 to drop the `bitcoin-core/gui` workaround. Updated the PR description to make clear that repo needs to have `NO_BRANCH=true` set _before_ merging.

https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652116935

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652754251)
Fixed `NO_ARM=true`

Joined the paragraphs.

Explained what happens when you do neither.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652754406)
Fixed.

I added an instruction at the top of this PR for where to set `NO_BRANCH=true`.

I can't find a Cirrus documentation page to point to for this, but you'll find it once you find the repo settings page.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652754504)
Done

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652754589)
Done

💬 paplorinc commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#discussion_r1652761548)
My bad, I assumed that since nothing was printed it was omitted, but
```
if (m_print_modified_fee) {
throw std::runtime_error("m_print_modified_fee is not implemented");
}
```
fails - thanks for pointing it out.