π¬ fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3536862669)
> although I would also consider peeling off bits->bytes refactor and span-usage to its own PR
It's a good idea, I will take it especially because I have been working on improved documentation for the asmap internals and this would go along nicely with this part of the original PR, I think. Since I started this restructuring yesterday already I will push this soon without your latest review here addressed: https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3448784895 (thank you f
...
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3536862669)
> although I would also consider peeling off bits->bytes refactor and span-usage to its own PR
It's a good idea, I will take it especially because I have been working on improved documentation for the asmap internals and this would go along nicely with this part of the original PR, I think. Since I started this restructuring yesterday already I will push this soon without your latest review here addressed: https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3448784895 (thank you f
...
π fjahr opened a pull request: "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation"
(https://github.com/bitcoin/bitcoin/pull/33878)
Depends on #33026, the prior part in this series. Please review this one first.
This is a second slice carved out of #28792. It contains the following changes that are crucial for the embedding of asmap data which is added the following PR in the series (probably this will remain in #28792).
The changes are:
- Modernizes and simplifies the asmap code by operating on `std::byte` instead of bits
- Unifies asmap version calculation and naming (previously it was called version and checksum i
...
(https://github.com/bitcoin/bitcoin/pull/33878)
Depends on #33026, the prior part in this series. Please review this one first.
This is a second slice carved out of #28792. It contains the following changes that are crucial for the embedding of asmap data which is added the following PR in the series (probably this will remain in #28792).
The changes are:
- Modernizes and simplifies the asmap code by operating on `std::byte` instead of bits
- Unifies asmap version calculation and naming (previously it was called version and checksum i
...
β οΈ fjahr opened an issue: "Embedded ASMap data Tracking Issue (Part 2)"
(https://github.com/bitcoin/bitcoin/issues/33879)
The predecessor tracking issue for this topic can be found here: #28794 This was closed since there seemed to be just one main PR left but I am reviving a topic issue for asmap because recently a few PRs have spun out of the main PR and related discussions. Aside from the somewhat tangential changes the main embedding PR is split into a series of three as of now.
- Embedding implementation series
- [ ] #33026
- [ ] #33857
- [ ] #28792
- [ ] https://github.com/bitcoin/bitcoin/issues/3338
...
(https://github.com/bitcoin/bitcoin/issues/33879)
The predecessor tracking issue for this topic can be found here: #28794 This was closed since there seemed to be just one main PR left but I am reviving a topic issue for asmap because recently a few PRs have spun out of the main PR and related discussions. Aside from the somewhat tangential changes the main embedding PR is split into a series of three as of now.
- Embedding implementation series
- [ ] #33026
- [ ] #33857
- [ ] #28792
- [ ] https://github.com/bitcoin/bitcoin/issues/3338
...
π€ 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.
(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
```
(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
...
(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
...
(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 :)
(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`.
(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.
(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.
(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
(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
...
(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
...
(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 *
...
(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://
...
(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)
(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.
(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?
(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.
(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.