📝 MarcoFalke opened a pull request: "ci: Add missing ${CI_RETRY_EXE} before curl"
(https://github.com/bitcoin/bitcoin/pull/28296)
GitHub is frequently down and this is causing many intermittent issues. For example, from today: https://cirrus-ci.com/task/5740122163904512?logs=ci#L398
Try to fix it with a retry.
(https://github.com/bitcoin/bitcoin/pull/28296)
GitHub is frequently down and this is causing many intermittent issues. For example, from today: https://cirrus-ci.com/task/5740122163904512?logs=ci#L398
Try to fix it with a retry.
💬 hebasto commented on pull request "ci: Add missing amd64 to win64-cross task":
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298383590)
Does the following log:
```
#3 [internal] load metadata for docker.io/library/ubuntu:jammy
```
look OK?
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298383590)
Does the following log:
```
#3 [internal] load metadata for docker.io/library/ubuntu:jammy
```
look OK?
👍 hebasto approved a pull request: "ci: Add missing ${CI_RETRY_EXE} before curl"
(https://github.com/bitcoin/bitcoin/pull/28296#pullrequestreview-1584467464)
ACK fa968ef6a32bb66ca9fa3b84807f1c960f6a9833
(https://github.com/bitcoin/bitcoin/pull/28296#pullrequestreview-1584467464)
ACK fa968ef6a32bb66ca9fa3b84807f1c960f6a9833
💬 MarcoFalke commented on pull request "ci: Add missing amd64 to win64-cross task":
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298389926)
Yes, the log looks green
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298389926)
Yes, the log looks green
💬 MarcoFalke commented on pull request "ci: Add missing amd64 to win64-cross task":
(https://github.com/bitcoin/bitcoin/pull/28295#issuecomment-1683842523)
Force pushed to do the same for android
(https://github.com/bitcoin/bitcoin/pull/28295#issuecomment-1683842523)
Force pushed to do the same for android
📝 Sjors converted_to_draft a pull request: "getblocktemplate improvements for segwit and sigops"
(https://github.com/bitcoin/bitcoin/pull/27433)
**Sigops**
Two recent F2Pool blocks violated the sigops limit. Both by 3.
I suspect they were not using `getblocktemplate`. If you look at the [template](https://miningpool.observer/template-and-block/00000000000000000004e0ec4f27bd3347381e8e19ed98d7f918e8c1c292ae97) right before the valid block at the same height, which was produced two minutes later, you'll see that it matches the block with 3 small transactions difference. In other words, the valid block producer likely did use `getblock
...
(https://github.com/bitcoin/bitcoin/pull/27433)
**Sigops**
Two recent F2Pool blocks violated the sigops limit. Both by 3.
I suspect they were not using `getblocktemplate`. If you look at the [template](https://miningpool.observer/template-and-block/00000000000000000004e0ec4f27bd3347381e8e19ed98d7f918e8c1c292ae97) right before the valid block at the same height, which was produced two minutes later, you'll see that it matches the block with 3 small transactions difference. In other words, the valid block producer likely did use `getblock
...
💬 hebasto commented on pull request "ci: Add missing amd64 to win64-cross task":
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298400895)
I was expecting to see:
```
#3 [internal] load metadata for docker.io/amd64/ubuntu:22.04
```
The PR description claims that this is a bugfix, but changes do not affect CI builds at all, according to the logs.
I'm confused a bit about that.
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298400895)
I was expecting to see:
```
#3 [internal] load metadata for docker.io/amd64/ubuntu:22.04
```
The PR description claims that this is a bugfix, but changes do not affect CI builds at all, according to the logs.
I'm confused a bit about that.
💬 MarcoFalke commented on pull request "ci: Add missing amd64 to win64-cross task":
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298404126)
It is possible to run the CI locally. For example, on a local `riscv64` machine you can type:
```
MAKEJOBS="-j1" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh
```
And then observe the failure.
The failure is fixed by this patch.
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298404126)
It is possible to run the CI locally. For example, on a local `riscv64` machine you can type:
```
MAKEJOBS="-j1" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh
```
And then observe the failure.
The failure is fixed by this patch.
💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#issuecomment-1683861802)
Rebased after #28244.
(https://github.com/bitcoin/bitcoin/pull/26567#issuecomment-1683861802)
Rebased after #28244.
💬 hebasto commented on pull request "ci: Add missing amd64 to win64-cross task":
(https://github.com/bitcoin/bitcoin/pull/28295#issuecomment-1683868538)
> Currently the task will fail if run on non-`x86_64`.
I expect it still fail with this PR with a non-`x86_64` Cirrus worker.
(https://github.com/bitcoin/bitcoin/pull/28295#issuecomment-1683868538)
> Currently the task will fail if run on non-`x86_64`.
I expect it still fail with this PR with a non-`x86_64` Cirrus worker.
💬 hebasto commented on pull request "ci: Add missing amd64 to win64-cross task":
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298411691)
> Currently the task will fail if run on non-`x86_64`.
I expect it still fail with this PR with a non-`x86_64` Cirrus worker.
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298411691)
> Currently the task will fail if run on non-`x86_64`.
I expect it still fail with this PR with a non-`x86_64` Cirrus worker.
💬 MarcoFalke commented on pull request "ci: Add missing amd64 to win64-cross task":
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298414958)
> I expect it still fail with this PR with a non-`x86_64` Cirrus worker.
No, it shouldn't, unless the switch is done incorrectly. If you switch to a non-`x86_64` Cirrus-hosted worker (you shouldn't do that), you should also change the docker label to match this one at the same time.
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298414958)
> I expect it still fail with this PR with a non-`x86_64` Cirrus worker.
No, it shouldn't, unless the switch is done incorrectly. If you switch to a non-`x86_64` Cirrus-hosted worker (you shouldn't do that), you should also change the docker label to match this one at the same time.
💬 fanquake commented on pull request "refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate":
(https://github.com/bitcoin/bitcoin/pull/28218#issuecomment-1683882791)
This will need to be rebased after #27460:
```bash
CXX rpc/libbitcoin_node_a-signmessage.o
CXX rpc/libbitcoin_node_a-txoutproof.o
rpc/mempool.cpp:757:28: error: no member named 'IsInitialBlockDownload' in 'Chainstate'
if (chainstate.IsInitialBlockDownload()) {
~~~~~~~~~~ ^
1 error generated.
gmake[2]: *** [Makefile:10829: rpc/libbitcoin_node_a-mempool.o] Error 1
```
(https://github.com/bitcoin/bitcoin/pull/28218#issuecomment-1683882791)
This will need to be rebased after #27460:
```bash
CXX rpc/libbitcoin_node_a-signmessage.o
CXX rpc/libbitcoin_node_a-txoutproof.o
rpc/mempool.cpp:757:28: error: no member named 'IsInitialBlockDownload' in 'Chainstate'
if (chainstate.IsInitialBlockDownload()) {
~~~~~~~~~~ ^
1 error generated.
gmake[2]: *** [Makefile:10829: rpc/libbitcoin_node_a-mempool.o] Error 1
```
🚀 fanquake merged a pull request: "ci: Add missing ${CI_RETRY_EXE} before curl"
(https://github.com/bitcoin/bitcoin/pull/28296)
(https://github.com/bitcoin/bitcoin/pull/28296)
💬 brunoerg commented on pull request "rpc, test: add `sendmsgtopeer` rpc and a test for net-level deadlock situation":
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298424645)
It works. But in the case it would be `target=node1.sendmsgtopeer` instead of `target=node1.send_message`.
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298424645)
It works. But in the case it would be `target=node1.sendmsgtopeer` instead of `target=node1.send_message`.
👍 MarcoFalke approved a pull request: "refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate"
(https://github.com/bitcoin/bitcoin/pull/28218#pullrequestreview-1584529348)
ACK ebda557e5ebe7f754984a34ddfd33d2540c0303b9 🦈
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK ebda557e5ebe7f754984a34dd
...
(https://github.com/bitcoin/bitcoin/pull/28218#pullrequestreview-1584529348)
ACK ebda557e5ebe7f754984a34ddfd33d2540c0303b9 🦈
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK ebda557e5ebe7f754984a34dd
...
💬 MarcoFalke commented on pull request "refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate":
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1298425994)
Same?
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1298425994)
Same?
💬 MarcoFalke commented on pull request "refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate":
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1298425415)
Can use `auto` to avoid using the same type twice on the same line?
```suggestion
auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
```
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1298425415)
Can use `auto` to avoid using the same type twice on the same line?
```suggestion
auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
```
💬 MarcoFalke commented on pull request "refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate":
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1298425581)
Same?
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1298425581)
Same?
👍 hebasto approved a pull request: "ci: Add missing amd64 to win64-cross task"
(https://github.com/bitcoin/bitcoin/pull/28295#pullrequestreview-1584534121)
ACK fa56d17a4b868f42fa45bea0d1f0c78de75e7838
(https://github.com/bitcoin/bitcoin/pull/28295#pullrequestreview-1584534121)
ACK fa56d17a4b868f42fa45bea0d1f0c78de75e7838
💬 fjahr commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1683891978)
> The onus in on you to show us that your PR will not break existing use cases.
His argument is, of course, that the use cases are already broken and thus the setting may as well be changed to reflect reality and help the network. I find the evidence presented convincing enough.
> If you don't think these use cases exist, go ahead and prove it.
Proving a negative in this context seems nearly impossible. How should he do that? Visit every website on the web and check if they accept 0-co
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1683891978)
> The onus in on you to show us that your PR will not break existing use cases.
His argument is, of course, that the use cases are already broken and thus the setting may as well be changed to reflect reality and help the network. I find the evidence presented convincing enough.
> If you don't think these use cases exist, go ahead and prove it.
Proving a negative in this context seems nearly impossible. How should he do that? Visit every website on the web and check if they accept 0-co
...