π¬ MarcoFalke commented on pull request "Refactor: Improve API design of `ScriptToUniv`, `TxToUniv` etc to return the `UniValue` instead of mutating a parameter/reference":
(https://github.com/bitcoin/bitcoin/pull/23507#discussion_r1221850091)
I think this is a bugfix for the fuzz test, which eats too much memory (especially under msan)
(https://github.com/bitcoin/bitcoin/pull/23507#discussion_r1221850091)
I think this is a bugfix for the fuzz test, which eats too much memory (especially under msan)
π¬ mjdietzx commented on pull request "Refactor: Improve API design of `ScriptToUniv`, `TxToUniv` etc to return the `UniValue` instead of mutating a parameter/reference":
(https://github.com/bitcoin/bitcoin/pull/23507#discussion_r1221863188)
Oh man I'm curious how you ended up back in this PR!? Is that just from your memory?
Anyways lmk if you want me to do anything. I'd be happy to rebase this, open a PR with just this commit, etc.. whatever you suggest. Also feel free to just cherry-pick that commit or whatever is most efficient for you as well incase you are just commenting this for documentation purposes
(https://github.com/bitcoin/bitcoin/pull/23507#discussion_r1221863188)
Oh man I'm curious how you ended up back in this PR!? Is that just from your memory?
Anyways lmk if you want me to do anything. I'd be happy to rebase this, open a PR with just this commit, etc.. whatever you suggest. Also feel free to just cherry-pick that commit or whatever is most efficient for you as well incase you are just commenting this for documentation purposes
π¬ glozow commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1221909729)
> Happy to add an assert, though
Maybe add this? `assert(!available_coins.empty());` above this line
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1221909729)
> Happy to add an assert, though
Maybe add this? `assert(!available_coins.empty());` above this line
π¬ MarcoFalke commented on pull request "doc: clarify full Xcode download is not needed":
(https://github.com/bitcoin/bitcoin/pull/27835#issuecomment-1581194624)
Not sure if this is needed? Anyone to copy-paste the command shouldn't run into issues and if you do random other stuff, you are on your own anyway?
(https://github.com/bitcoin/bitcoin/pull/27835#issuecomment-1581194624)
Not sure if this is needed? Anyone to copy-paste the command shouldn't run into issues and if you do random other stuff, you are on your own anyway?
π¬ MarcoFalke commented on pull request "Refactor: Improve API design of `ScriptToUniv`, `TxToUniv` etc to return the `UniValue` instead of mutating a parameter/reference":
(https://github.com/bitcoin/bitcoin/pull/23507#discussion_r1221913157)
Maybe just split out the `TxToUniv` change in a new pull?
(https://github.com/bitcoin/bitcoin/pull/23507#discussion_r1221913157)
Maybe just split out the `TxToUniv` change in a new pull?
π¬ ryanofsky commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1581200866)
Approach ACK d44dd73bcabc694880dfc966fad21980cd3b4c64. Need to review more closely, but these changes look very good and should help with #27711
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1581200866)
Approach ACK d44dd73bcabc694880dfc966fad21980cd3b4c64. Need to review more closely, but these changes look very good and should help with #27711
π¬ jarolrod commented on pull request "ci: Nuke Android APK task, Use credits for tsan":
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1581305041)
Fine with me to repurpose to something more useful for this repo π
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1581305041)
Fine with me to repurpose to something more useful for this repo π
π¬ jarolrod commented on pull request "ci: Nuke Android APK task, Use credits for tsan":
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1581325077)
It should be noted the primary role of the task is to check that a change doesnβt prevent us from building for android, a platform with 3 billion users and a platform that we care about building for. A manual check will be needed for build system changes to ensure that this compatibility is kept.
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1581325077)
It should be noted the primary role of the task is to check that a change doesnβt prevent us from building for android, a platform with 3 billion users and a platform that we care about building for. A manual check will be needed for build system changes to ensure that this compatibility is kept.
π¬ willcl-ark commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1581330720)
> Not sure what is up with CI, but lint says:
>
> ```
> contrib/devtools/special-instruction-check.py:55: error: "Binary" has no attribute "has_section" [attr-defined]
> ```
>
> Maybe rebase?
mypy decided it didn't like that line. Got rid of it with `# type: ignore[attr-defined]` as I don't know any better
Did the CI randomly re-run on this?
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1581330720)
> Not sure what is up with CI, but lint says:
>
> ```
> contrib/devtools/special-instruction-check.py:55: error: "Binary" has no attribute "has_section" [attr-defined]
> ```
>
> Maybe rebase?
mypy decided it didn't like that line. Got rid of it with `# type: ignore[attr-defined]` as I don't know any better
Did the CI randomly re-run on this?
π¬ fanquake commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1581333578)
> Got rid of it with
There shouldn't be any need for changes, as that issue was fixed with upgrading to LIEF 0.13.1. So should be resolved after rebasing.
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1581333578)
> Got rid of it with
There shouldn't be any need for changes, as that issue was fixed with upgrading to LIEF 0.13.1. So should be resolved after rebasing.
π furszy opened a pull request: "rpc: getblockfrompeer, introduce fetch from "any" functionality"
(https://github.com/bitcoin/bitcoin/pull/27836)
Coming from #27652. Implementing the first part of it.
The idea is to let users call `getblockfrompeer` without providing any peer id.
The node will internally select one peer at random and make the `getdata` request.
This also fixes a bug where the user is allowed to call `getblockfrompeer` providing
an id of a peer that signals a "limited" service. As limited peers cannot provide historical
blocks, it is not correct to allow the user to do that.
(https://github.com/bitcoin/bitcoin/pull/27836)
Coming from #27652. Implementing the first part of it.
The idea is to let users call `getblockfrompeer` without providing any peer id.
The node will internally select one peer at random and make the `getdata` request.
This also fixes a bug where the user is allowed to call `getblockfrompeer` providing
an id of a peer that signals a "limited" service. As limited peers cannot provide historical
blocks, it is not correct to allow the user to do that.
π¬ willcl-ark commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1581334980)
Ah I see. I rebased an re-ran the lint to see it fail locally, but didn't update my lief version!
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1581334980)
Ah I see. I rebased an re-ran the lint to see it fail locally, but didn't update my lief version!
π¬ willcl-ark commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1581339396)
It does in fact, appear to need changes:
```log
will@ubuntu in ~/src/bitcoin-18603_si_check on ξ 18603_si_check [$!?] : C v16.0.5-clang : π 3.8.16
$ pip3 install lief==0.13.1
Collecting lief==0.13.1
Using cached lief-0.13.1-cp38-cp38-manylinux_2_24_x86_64.whl (4.1 MB)
Installing collected packages: lief
Successfully installed lief-0.13.1
will@ubuntu in ~/src/bitcoin-18603_si_check on ξ 18603_si_check [$!?] : C v16.0.5-clang : π 3.8.16
$ ./test/lint/all-lint.py
src/addrdb.cpp:21
...
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1581339396)
It does in fact, appear to need changes:
```log
will@ubuntu in ~/src/bitcoin-18603_si_check on ξ 18603_si_check [$!?] : C v16.0.5-clang : π 3.8.16
$ pip3 install lief==0.13.1
Collecting lief==0.13.1
Using cached lief-0.13.1-cp38-cp38-manylinux_2_24_x86_64.whl (4.1 MB)
Installing collected packages: lief
Successfully installed lief-0.13.1
will@ubuntu in ~/src/bitcoin-18603_si_check on ξ 18603_si_check [$!?] : C v16.0.5-clang : π 3.8.16
$ ./test/lint/all-lint.py
src/addrdb.cpp:21
...
β
willcl-ark closed a pull request: "build: special instruction check script"
(https://github.com/bitcoin/bitcoin/pull/26693)
(https://github.com/bitcoin/bitcoin/pull/26693)
π willcl-ark reopened a pull request: "build: special instruction check script"
(https://github.com/bitcoin/bitcoin/pull/26693)
Fixes #18603
Checks that generated files with special compilation units do not contain disallowed sections. Special instructions and disallowed sections are defined in the script and include `SSE42`, `SSE41`, `AVX`, `AVX2`, `SHANI`, and `.text.startup`, respectively.
The script uses `glob` to search and `lief` to parse files. If any disallowed sections are found in a file the script will print an error to `stderr` and exit with a non-zero exit code. Otherwise, the script will exit with a
...
(https://github.com/bitcoin/bitcoin/pull/26693)
Fixes #18603
Checks that generated files with special compilation units do not contain disallowed sections. Special instructions and disallowed sections are defined in the script and include `SSE42`, `SSE41`, `AVX`, `AVX2`, `SHANI`, and `.text.startup`, respectively.
The script uses `glob` to search and `lief` to parse files. If any disallowed sections are found in a file the script will print an error to `stderr` and exit with a non-zero exit code. Otherwise, the script will exit with a
...
π furszy opened a pull request: "net: introduce block tracker to retry to download blocks after failure"
(https://github.com/bitcoin/bitcoin/pull/27837)
Built on top of #27836. Coming from #27652. Implementing the second part of it.
The general idea is to keep track of the user requested blocks so, in
case of a bad behaving peer or a network disconnection, they can be
fetched from another one automatically without any further user interaction.
This was requested by users because the `getblockfrompeer` RPC command
lacks the functionality to notify them about block request failures or peer
disconnections (which is expected due to the asy
...
(https://github.com/bitcoin/bitcoin/pull/27837)
Built on top of #27836. Coming from #27652. Implementing the second part of it.
The general idea is to keep track of the user requested blocks so, in
case of a bad behaving peer or a network disconnection, they can be
fetched from another one automatically without any further user interaction.
This was requested by users because the `getblockfrompeer` RPC command
lacks the functionality to notify them about block request failures or peer
disconnections (which is expected due to the asy
...
π¬ hebasto commented on pull request "guix: Clean up manifest":
(https://github.com/bitcoin/bitcoin/pull/27811#discussion_r1222074702)
> Probably all the other cases should be using `url-fetch` instead, or at least with the exception of the sourceware repository?
It seems the opposite approach is preferable according to [that](https://lists.gnu.org/archive/html/guix-devel/2020-05/msg00224.html) discussion. Anyway, I'm happy to drop that commit if it is indeed controversial.
(https://github.com/bitcoin/bitcoin/pull/27811#discussion_r1222074702)
> Probably all the other cases should be using `url-fetch` instead, or at least with the exception of the sourceware repository?
It seems the opposite approach is preferable according to [that](https://lists.gnu.org/archive/html/guix-devel/2020-05/msg00224.html) discussion. Anyway, I'm happy to drop that commit if it is indeed controversial.
π¬ brunoerg commented on pull request "doc: clarify full Xcode download is not needed":
(https://github.com/bitcoin/bitcoin/pull/27835#discussion_r1222100182)
Seems weird to say "you don't need to install" and the next line "To install, run...". If we want to advertise something, I prefer something like: "Recent MacOS versions may contain Xcode by default"
(https://github.com/bitcoin/bitcoin/pull/27835#discussion_r1222100182)
Seems weird to say "you don't need to install" and the next line "To install, run...". If we want to advertise something, I prefer something like: "Recent MacOS versions may contain Xcode by default"
π¬ willcl-ark commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1581496357)
It seems that although the stubs can be found in https://github.com/lief-project/LIEF/blob/b9a5f970b210dbc115c7aaac10e04623bdc08ef8/api/python/lief/ELF.pyi#L369-L371, they are not present for the `Binary` type https://github.com/lief-project/LIEF/blob/b9a5f970b210dbc115c7aaac10e04623bdc08ef8/api/python/lief/__init__.pyi#L27.
It almost looked possible that we could just switch to `lief.ELF.parse(file)` instead, but this _also_ returns a `Binary` on the Python API:
> We start by the ELF form
...
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-1581496357)
It seems that although the stubs can be found in https://github.com/lief-project/LIEF/blob/b9a5f970b210dbc115c7aaac10e04623bdc08ef8/api/python/lief/ELF.pyi#L369-L371, they are not present for the `Binary` type https://github.com/lief-project/LIEF/blob/b9a5f970b210dbc115c7aaac10e04623bdc08ef8/api/python/lief/__init__.pyi#L27.
It almost looked possible that we could just switch to `lief.ELF.parse(file)` instead, but this _also_ returns a `Binary` on the Python API:
> We start by the ELF form
...
π ryanofsky approved a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1468674496)
Code review ACK d44dd73bcabc694880dfc966fad21980cd3b4c64. But it would be nice to extend this change to work with bitcoin-qt as well. I also think it would be good to apply the following changes, which replace the `bool shutdown_due_error` global with an `int exit_status{EXIT_SUCCESS}` `NodeContext` member.
<details><summary>diff</summary>
<p>
```diff
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index c305d5e6cdc1..36a13aded666 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
...
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1468674496)
Code review ACK d44dd73bcabc694880dfc966fad21980cd3b4c64. But it would be nice to extend this change to work with bitcoin-qt as well. I also think it would be good to apply the following changes, which replace the `bool shutdown_due_error` global with an `int exit_status{EXIT_SUCCESS}` `NodeContext` member.
<details><summary>diff</summary>
<p>
```diff
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index c305d5e6cdc1..36a13aded666 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
...