Bitcoin Core Github
44 subscribers
121K links
Download Telegram
πŸ€” furszy reviewed a pull request: "qa: Account for unset errno in ConnectionResetError"
(https://github.com/bitcoin/bitcoin/pull/33875#pullrequestreview-3468346292)
Tested ACK 76e0e6087d0310ec31f43d751de60adf0c0a2faa

Thanks for improving this. It makes understanding internal issues simpler.

It’d be nice if the upcoming http server implementation caught these unhandled exceptions, writes to `std::cerr` and returns a proper error instead of exposing the internal one via the http response. Tagging @pinheadmz.
πŸ’¬ furszy commented on pull request "qa: Account for unset errno in ConnectionResetError":
(https://github.com/bitcoin/bitcoin/pull/33875#discussion_r2530131801)
nit: might want to update the line below to state this could also happen due to an uncaught exception at the server side. Talking about this:
```python
errno.ECONNRESET, # This might happen when the RPC server is in warmup
```
πŸ’¬ fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3536888674)
I am taking this PR back into draft status temporarily because I have been slicing it into 3 parts thanks to the suggestions of @hodlinator . The first part is #33026 which features some minor prep work and is ready for review right now. The second part is #33878 which can be reviewed now but has not addressed the [latest feedback from today](https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3448784895). There is one new commit in there which I hope is interesting: it adds further
...
πŸ“ fjahr converted_to_draft a pull request: "Embedded ASMap [3/3]: Build binary dump header file"
(https://github.com/bitcoin/bitcoin/pull/28792)
This is a step towards making asmap usage more accessible. Currently an asmap file needs to be acquired by there user from some location or the user needs to generate one themselves. Then they need to move the file to the right place in datadir or pass the path to the file as `-asmap=PATH` in order to use the asmap feature.

The change here allows for builds to embed asmap data into the bitcoind binary which makes it possible to use the feature without handling of the asmap file by the user. I
...
πŸ’¬ fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3536890738)
@sipa would love to get your eyes on the [documentation commit](https://github.com/bitcoin/bitcoin/pull/33878/commits/93205dd90f525a42f9e41cadb289c4075c5fca09) before anyone else gets confused by the mistakes I might have made :)
πŸ’¬ fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2530154158)
I can't remember encountering such a tool but primarily I was going by our style guide, which I thought had mentioned that we don't do new lines before EOF but I guess it doesn't (anymore?) since I couldn't find it right now. At least we have `InsertNewlineAtEOF: false` in our `.clang-format`.
πŸ’¬ sipa commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2530246100)
All the documentation changes in this commit look correct to me.

One thing that may be worth mentioning early on is that it's a bit-packed format, i.e., the entire compressed mapping is treated as a sequence of bits, packed together at 8 per byte, which are concatenated encodings of instructions.
πŸ“ fjahr opened a pull request: "test: Fix race condition in IPC interface block progation test"
(https://github.com/bitcoin/bitcoin/pull/33880)
CI failed on this condition here: https://github.com/bitcoin/bitcoin/actions/runs/19395398994/job/55494696022?pr=33878#step:9:3983

The check was added not too long ago in https://github.com/bitcoin/bitcoin/pull/33745 and it seems like an easy fix to wait a bit but I am not sure if the check could also not just be removed. Afaict it doesn't make a difference whether the regression below it is hit or not.
πŸ’¬ fjahr commented on pull request "test: Fix race condition in IPC interface block progation test":
(https://github.com/bitcoin/bitcoin/pull/33880#issuecomment-3536959684)
cc: @Sjors
πŸ“ thelonestar63 opened a pull request: "Snailcoin rebrand"
(https://github.com/bitcoin/bitcoin/pull/33881)
<!--
*** 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 or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...
⚠️ roconnor-blockstream opened an issue: "Standardness policy rules for legacy Multisig script is incoherent"
(https://github.com/bitcoin/bitcoin/issues/33882)
Over the years policy surrounding multisig scripts, particularly for legacy multisig scripts, has evolved in a haphazard way with various PRs affecting legacy multisig script policy without considering the ramifications.

The current status of legacy multisig script policy is as follows:

* Legacy multisig outputs in transactions can be at most *m*-of-3. All pubkeys in such outputs must be of the form of a compressed pubkey, and uncompressed pubkey, or a hybrid pubkey.
* Legacy multisig inputs
...
πŸ’¬ roconnor-blockstream commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3537392532)
## How did we get here

I've compiled a list of what I believe are all the relevant PRs related to policy surrounding multisig scripts, listed in chronological order.
This work is based on previous work done by @ajtowns

### PR #669; merged 2011-12-20
This PR introduces *m*-of-*n* checkmultisig as a solvable script for the first time.
Solvability requires *m* and *n* to be at most 16.
Per BIP 11, standardness requires solvable MULTISIG tx outputs to be at most *m*-of-3 and *m* to be less than *
...
⚠️ ofry opened an issue: ""Send" text field has too little amount"
(https://github.com/bitcoin/bitcoin/issues/33883)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

The "Amount" input field on the "Send" tab is too narrow; the user can only see a single digit.

The screenshot is below:

<img width="1280" height="1024" alt="Image" src="https://github.com/user-attachments/assets/2c75a951-609d-4c87-b62e-8aa3d31494cc" />

### Expected behaviour

User should see all digits on payment amount.

### Steps to reproduce

1. Download latest release from https://
...
πŸ’¬ yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3537983534)
Thanks for the review @TheCharlatan
- Removed some unnecessary comments and indentation
- Added inline doc [comment](https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2528773169)
πŸ€” yuvicc reviewed a pull request: "kernel: Rename in-memory DB option setters and simplify API"
(https://github.com/bitcoin/bitcoin/pull/33877#pullrequestreview-3469677908)
Concept ACK

Makes sense to refractor update to set and remove the int parameter here.
πŸ’¬ yuvicc commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#discussion_r2531456075)
I think these options are set by default?
πŸ’¬ yuvicc commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2531493620)
The problem is that `ActivateBestChain` would overwrite the `state` result, when the background chain's `ActivateBestChain` succeeds, it would overwrite state with its own `state`. Then at line 4554, we'd be returning the background chain's `state` instead of the active chainstate's `state`. So by separating out, we preserve the active chainstate result.
πŸ’¬ yuvicc commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2531525257)
Correct, we will definitely need that.
πŸ’¬ yuvicc commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#issuecomment-3538243414)
Thanks for the review @hodlinator
- Added `Assume(!headers.empty())` before instantiating validation state as suggested in [comment](https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517354777)
- Removed shadow `state` [comment](https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517333716)
⚠️ yuvicc opened an issue: "interface_ipc functional test failing in CI"
(https://github.com/bitcoin/bitcoin/issues/33884)
## Summary

The `interface_ipc` functional test is failing in CI but passes when run locally.

Affected PR's: #33856 and #33878