Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows, OpenBSD)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2622194575)
#31740 landed, now trying on top #31741.
⚠️ ryanofsky opened an issue: "RFC: multiprocess binaries in 29.0"
(https://github.com/bitcoin/bitcoin/issues/31756)
I want to reopen topic of including multiprocess binaries in [29.0](https://github.com/bitcoin/bitcoin/issues/31029). I'm fine with any decision on this issue, but I was rereading the previous [irc discussion](https://www.erisian.com.au/bitcoin-core-dev/log-2025-01-23.html#l-303) and it struck me as abstract and process-oriented, detached from reality, and likely based on misperceptions of the actual change being discussed.

I think it would be helpful if people who had concerns about including
...
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows, OpenBSD)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2622362658)
I dropped eb41f92c0889708045ce0a30c6d0f8d43cecd6d5 if favor of setting `MULTIPROCESS=1` for most depends jobs (9a20dc4f2ebbb4179dfabd6426aa442069649907).

Once https://github.com/bitcoin/bitcoin/pull/31741 lands (and this build succeeds) I'll probably open a fresh PR to avoid confusion.
💬 ryanofsky commented on issue "RFC: multiprocess binaries in 29.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2622374479)
Responding to fanquake's [post](https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2610191420) a few minutes minutes before the meeting which was very useful and constructive, but also one sided and I think did not result in a balanced discussion at the meeting.

> Given that merging this PR for `29.x` is going to be discussed further, I wanted to leave a few general thoughts here.
>
> #### The mining interface isn't finished yet?
> Looking at #31098, there are still un-merged changes in
...
👍 tdb3 approved a pull request: "rpc: have getblocktemplate mintime account for timewarp"
(https://github.com/bitcoin/bitcoin/pull/31600#pullrequestreview-2581759004)
brief code review re ACK e1676b08f7b0b9a6c8ed76e31f24faa03a3facc9

Most significant change observed was to "always apply"
🤔 marcofleon reviewed a pull request: "test: fix intermittent timeout in p2p_1p1c_network.py"
(https://github.com/bitcoin/bitcoin/pull/31751#pullrequestreview-2581769938)
ACK 152a2dcdefa6ec744db5b106d5c0a8c5b8aca416

iiuc, when we disconnect, this clears any 60s requests for parents that node1 has with the initial setup peers. So now there's no chance the `sync_mempools()` timeout is hit before the parent request timeout.
🤔 glozow reviewed a pull request: "test: fix intermittent timeout in p2p_1p1c_network.py"
(https://github.com/bitcoin/bitcoin/pull/31751#pullrequestreview-2581783125)
reACK 152a2dcdefa6ec744db5b106d5c0a8c5b8aca416
💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2622421647)
Just saw #31756, but even if multiprocess does make it into the v29 release, it seems better at this point to switch the default in a separate PR. And also to not have the changes here be blocked by ongoing discussion.
💬 Sjors commented on issue "RFC: multiprocess binaries in 29.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2622442953)
Sorry about changing the title and scope of #30975 from under you right before you opened this. It now only changes the CI to run multiprocess everywhere and avoids enabling it by default (including it in the release, since this follows what's in depends). But the latter of course is what the main discussion is about.
💬 darosior commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2622444828)
> Maybe you can be more concrete and say what negative outcome(s) you would like to avoid?

Sure, i meant i don't see why we should release non-IPC binaries once we have decided to enable multiprocess. It seems it would make us be in this halfway situation of keeping around the bigger binaries with no clear reason nor timeline about when we'd get rid of them.
💬 ryanofsky commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2622479743)
> Sure, i meant i don't see why we should release non-IPC binaries once we have decided to enable multiprocess.

Got it. I honestly don't have an opinion about that. I think existing non-ipc binaries should be kept as long as features like adding a Chain IPC interface (#29409) or removing wallet code from `bitcoin-node` process (#10102) are planned. But after that point you could easily drop them, especially if #31375 gained adoption. You could also remove them in phases, like (1) move them to l
...
💬 darosior commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2622488749)
I agree the multiprocess project should be finished before we consider enabling it and shipping binaries. I guess that ties into the discussion in #31756 so i'll expand there.
📝 furszy opened a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757)
Before detailing the issue:
This crash should not occur in production, as block events are queued and processed in
order by the validation signal workers. This PR is just fixing brittle code.

Issue:
The wallet crashes if it processes the same block disconnection event twice in a row due
to an incompatible coinbase transaction state.
This happens because `disconnectBlock` provides `TxStateInactive` without the "abandoned"
flag for coinbase transactions to `SyncTransaction`, while `AddToW
...
📝 sr-gi opened a pull request: "test: deduplicates p2p_tx_download constants"
(https://github.com/bitcoin/bitcoin/pull/31758)
Some of the networking constants defined in p2p_tx_download.py are more generally defined in p2p.py

<!--
*** 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 o
...
💬 darosior commented on issue "RFC: multiprocess binaries in 29.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2622618673)
I'm not sure i'm the most legitimate to give my opinion here, but since you asked me explicitly i will share.

First of all why is the default position that we should ship a new binary on short notice right before feature freeze, and people who are not comfortable with this should argue against it? It seems backward to me. The default should be business as usual and if people want to introduce new binaries for the project to release, they should be arguing for it.

If you start from this default
...
📝 sr-gi opened a pull request: "test: fixes p2p_ibd_txrelay wait time"
(https://github.com/bitcoin/bitcoin/pull/31759)
`p2p_ibd_txrelay` expects no GETDATA to have been received by a peer after announcing a transaction. The reason is that the node is doing IBD, so transaction requests are not replied to. However, the way this is checked is wrong, and the check will pass even if the node **was not** in IBD.

This is due to the mocktime not being properly initialized, so the check is always performed earlier than it should, making it impossible for the request to be there.

This can be checked by modifying the
...
🤔 hodlinator reviewed a pull request: "miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`"
(https://github.com/bitcoin/bitcoin/pull/31734#pullrequestreview-2581415219)
Code review 0bc213db7686df9ccbd5ef76c5569181851d80a2

Thanks for taking on this issue! Not very familiar with this area but maybe I can assist in refining the PR anyway. Seems directionally correct to use `h` consistently for hardened derivation paths unless backwards compatibility is requested.

### PR title/summary

#### PR title typo
acount -> account

#### Summary

```diff
- In `MiniscriptDescriptor::ToStringHelper()` only `StringType::Private` variant of `type` argument was acco
...
💬 hodlinator commented on pull request "miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1934112560)
nit: Could keep style with rest of the fields:
```suggestion
const DescriptorImpl::StringType m_type;
```
💬 hodlinator commented on pull request "miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1934100346)
In commit 0bc213db7686df9ccbd5ef76c5569181851d80a2:
This change should be part of the other commit.
💬 hodlinator commented on pull request "miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1934103988)
Seems okay to store as raw pointer, if we take it as such as an argument into the `StringMaker` constructor?
```suggestion
const DescriptorCache* m_cache;
```