💬 viresinnumeris-1 commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1683822349)
@petertodd

Repeatedly seeking "citations" for 0-conf use cases is not the way to convince people that your PR should be merged. You made the PR, seeking to change a long-standing default value. The onus in on **you** to show us that your PR will not break existing use cases. Forcefully demanding users and supporters of 0-conf/FSS policies to show their data is fallacious shifting of burden of proof. If you don't think these use cases exist, go ahead and prove it.

Your own views on mempool
...

💬 hebasto commented on pull request "ci: Add missing amd64 to win64-cross task":
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298378816)
Same change in `cirrus.sh`?

💬 MarcoFalke commented on pull request "ci: Add missing amd64 to win64-cross task":
(https://github.com/bitcoin/bitcoin/pull/28295#discussion_r1298380304)
In theory yes, but in practise, no, because:

* Cirrus already runs on amd64, so this won't change anything.
* The config will be removed in another pull, so changing it will cause merge conflicts.

📝 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.

💬 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?

👍 hebasto approved a pull request: "ci: Add missing ${CI_RETRY_EXE} before curl"
(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

💬 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

📝 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
...

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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.

💬 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
```

🚀 fanquake merged a pull request: "ci: Add missing ${CI_RETRY_EXE} before curl"
(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`.

👍 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
...

💬 MarcoFalke commented on pull request "refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate":
(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);
```