💬 fanquake commented on pull request "macdeploy: avoid use of `Bitcoin Core` in Linux cross build":
(https://github.com/bitcoin/bitcoin/pull/33158#issuecomment-3242773819)
> I find it weird that we would produce files named differently for the same target when the hosts are different
> Agreed that different names depending on originating platform might be a bit confusing.

I don't mind much, but I don't think the different names are that weird, given the two outputs are different. One is a release-like zip, with an immediately usable binary, the other is a zip containing a binary that wont run, which is immediately renamed, and fed into the codesigning process.
...

💬 151henry151 commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3242805794)
Is there anything further required here? I understand that review for pull requests can take a long time (especially for non-critical things such as this one) but just wanted to check in and see if there's further action needed from me, or anything about this pull request that I could improve.

💬 fanquake commented on pull request "build: add `-Wleading-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#discussion_r2314275670)
Sure. https://github.com/arun11299/cpp-subprocess/pull/121.

💬 fanquake commented on pull request "build: add `-Wleading-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#discussion_r2314284822)
https://github.com/bitcoin-core/minisketch/pull/98.

💬 fanquake commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2314285687)
If we're going to call the scripts directly, then you should remove the targets from cmake, as there's no point having them, if they aren't used in the Guix build.

✅ fanquake closed a pull request: "cmake: Use `AUTHOR_WARNING` for warnings"
(https://github.com/bitcoin/bitcoin/pull/32865)

💬 fanquake commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3242839579)
Maybe CMake will provide an option to properly do this in future, but not going to work on this further.

🤔 janb84 reviewed a pull request: "ci: Migrate CI to hosted Cirrus Runners"
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3174188672)
re ACK 3c5da69a232ba1cfb935012aa53e57002efe0d77

changes since last ACK:
- changed cirrus-runner for 32 bit ARM

💬 Sjors commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#issuecomment-3242963481)
ACK 01fbbc5b7ea02c98f54255b8dba342a065edb9f7

I didn't check the datadir sizes.

I tested loading the testnet4 snapshot and then syncing to the tip. Still syncing a mainnet node with `-assumevalid=0` (also using a loaded snapshot), will update here in the unlikely event that it fails.

💬 151henry151 commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2314390992)
> If we're going to call the scripts directly, then you should remove the targets from cmake, as there's no point having them, if they aren't used in the Guix build.

Thanks @fanquake -- I believe I've correctly removed the targets from cmake now in commit ef5dd15309f9.
The Guix build hashes:

b7775f39fab10c11713ca7bf38a8cac48a143a9478999ee64b927ea9c5040245 bitcoin-ef5dd15309f9.tar.gz
e79d2bd8072a459eb75db8ef06f41a5dd9128ad63ca308cdf7c57c39670eb99f bitcoin-ef5dd15309f9-x86_64-linux-gnu.t
...

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2314389493)
I’m not exactly sure how this works; my IDEs break here. Is my understanding correct that we’re just piggybacking a new value onto a `Future`’s per-instance `__dict__`?
Would it be cleaner to use a Future-to-string dictionary instead?
```python
self.job_name = {}
...
future = self.executor.submit(proc_wait, task)
self.job_name[future] = test
self.jobs.append(future)
...
print("Remaining jobs: [{}]".format(", ".join(self.job_name[f] for f in self.jobs)))
```
(I checked, and it seems di
...

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2314417456)
is it still correct to claim that we're sleeping?

👍 l0rinc approved a pull request: "test: Remove polling loop from test_runner (take 2)"
(https://github.com/bitcoin/bitcoin/pull/33141#pullrequestreview-3174274620)
tested ACK fa7a3b5fb04465a3fbadf1ca62eac26827c1ef1d

there are still a few things I'd prefer before we merge, but won't block

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2314392659)
nit: since this is new code, maybe we could decompose `done` and `not_done` here already
```python
done, not_done = futures.wait(self.jobs, timeout=.5, return_when=futures.FIRST_COMPLETED)
self.jobs = list(not_done)
ret = []
for fut in done:
```

💬 l0rinc commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-3243043898)
Of course, paused it since I'm not exactly sure how do handle single-byte entries: they measurably speed up the scenarios, but the resulting code is quite ugly. Do you have a suggestion? Should I attempt that in a separate PR and ignore it here?

💬 l0rinc commented on pull request "coins,refactor: Reduce `getblockstats` RPC UTXO overhead estimation":
(https://github.com/bitcoin/bitcoin/pull/31449#discussion_r2314439240)
yes, both are over-estimating, but this is closer to the actual functioning, the previous one explicitly adds the bool which isn't separate.
We can also just fix it by not making this a `constexpr` but a function of the actual values, but I'd argue the current versions is still better.

💬 l0rinc commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243067816)
> we are not using ID-based translations

@hebasto, are suggesting that the bitcoin-test clone where we tried it isn't representative? It did seem to me that we managed to reproduce and fix the instability by stabilizing the ids. But if we're not using the ids, why aren't the translations stable? What alternative do you suggest to stabilize them?

> shasum of the full content can be used as an id for the translation string

@maflcko we could of course do that, but it would likely invalidat
...

💬 ismaelsadeeq commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3243084444)
https://cirrus-ci.com/task/6512217929023488?logs=ci#L3103

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-3243087856)
C.I Failure is not related see https://github.com/bitcoin/bitcoin/issues/33244

💬 l0rinc commented on pull request "build: add `-Wleading-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3243095040)
ACK b271b342e36000952a389c05b409bab691a4e4d2