✅ 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
...
💬 ajtowns commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1581674023)
re ACK 686629d2be5545ef59cf0e97f4f3a74c6cde2efa
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1581674023)
re ACK 686629d2be5545ef59cf0e97f4f3a74c6cde2efa
🤔 jarolrod reviewed a pull request: "ci: Nuke Android APK task, Use credits for tsan"
(https://github.com/bitcoin/bitcoin/pull/27834#pullrequestreview-1468894928)
NACK
I'm not sure if the PR description counts as "many" issues.
If this task was failing intermittently all the time, why are there no issues posted about these intermittent failures occurring all the time, everywhere? We watch this task often in the QML repo and have never had it fail due to a network timeout.
It's great to speed up a task, and I wish all tasks could run as quickly as possible; that would be really great. But not at the expense of a check on building for a platform we
...
(https://github.com/bitcoin/bitcoin/pull/27834#pullrequestreview-1468894928)
NACK
I'm not sure if the PR description counts as "many" issues.
If this task was failing intermittently all the time, why are there no issues posted about these intermittent failures occurring all the time, everywhere? We watch this task often in the QML repo and have never had it fail due to a network timeout.
It's great to speed up a task, and I wish all tasks could run as quickly as possible; that would be really great. But not at the expense of a check on building for a platform we
...
💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1581806840)
> * The potential downside is clear: by default, bitcoind considers both IPv4 & IPv6 to be reachable. However, both Martin & I have experienced running nodes within a network setup such that only connection attempts to IPv4 nodes were successful. A node in this environment running current master will still have failed IPv6 connection attempts. But the problem (aka frequency of failed attempts) would increase in severity if we added this patch with logic that treated IPv4 & IPv6 separately.
I'
...
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1581806840)
> * The potential downside is clear: by default, bitcoind considers both IPv4 & IPv6 to be reachable. However, both Martin & I have experienced running nodes within a network setup such that only connection attempts to IPv4 nodes were successful. A node in this environment running current master will still have failed IPv6 connection attempts. But the problem (aka frequency of failed attempts) would increase in severity if we added this patch with logic that treated IPv4 & IPv6 separately.
I'
...
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1222386277)
yeah, the reason for this function signature was to enable calling from within the `else if(...)` conditional. pulling it out would mean having to run `MaybePickPreferredNetwork` every time `ThreadOpenConnections` runs, which seems unnecessary?
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1222386277)
yeah, the reason for this function signature was to enable calling from within the `else if(...)` conditional. pulling it out would mean having to run `MaybePickPreferredNetwork` every time `ThreadOpenConnections` runs, which seems unnecessary?
💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1222388434)
I think the requirement here is only "try each network with reasonable probability" -- you want to be connected to all of the networks, so which one comes first doesn't matter very much.
If you don't have ipv4 or ipv6 connections but are going through this codepath, then that means you have all your outbounds connected via privacy networks, but haven't chosen to `-onlynet=tor` or similar. So biassing towards clearnet probably makes sense anyway: clearnet connections probably have higher bandw
...
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1222388434)
I think the requirement here is only "try each network with reasonable probability" -- you want to be connected to all of the networks, so which one comes first doesn't matter very much.
If you don't have ipv4 or ipv6 connections but are going through this codepath, then that means you have all your outbounds connected via privacy networks, but haven't chosen to `-onlynet=tor` or similar. So biassing towards clearnet probably makes sense anyway: clearnet connections probably have higher bandw
...
💬 MarcoFalke commented on pull request "ci: Nuke Android APK task, Use credits for tsan":
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1581873413)
> why are there no issues posted about these intermittent failures
Because no one reported them. But they do exist, if you grep for "Warning: An error occurred while preparing SDK package Android Emulator: Connection reset." For example, https://cirrus-ci.com/task/5766445213155328?logs=build#L2700
> It's great to speed up a task, and I wish all tasks could run as quickly as possible;
It only checks that it can build, and doesn't run any tests, so I hope the other tasks remain to run the
...
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1581873413)
> why are there no issues posted about these intermittent failures
Because no one reported them. But they do exist, if you grep for "Warning: An error occurred while preparing SDK package Android Emulator: Connection reset." For example, https://cirrus-ci.com/task/5766445213155328?logs=build#L2700
> It's great to speed up a task, and I wish all tasks could run as quickly as possible;
It only checks that it can build, and doesn't run any tests, so I hope the other tasks remain to run the
...
🤔 MarcoFalke reviewed a pull request: "fuzz: wallet, add target for `fees`"
(https://github.com/bitcoin/bitcoin/pull/27647#pullrequestreview-1468982852)
lgtm ACK 4da8d6bf17c0875c1a8f60ad2bb1bbd418d3a7cd
left some nits, didn't test
(https://github.com/bitcoin/bitcoin/pull/27647#pullrequestreview-1468982852)
lgtm ACK 4da8d6bf17c0875c1a8f60ad2bb1bbd418d3a7cd
left some nits, didn't test
💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1222442326)
not needed?
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1222442326)
not needed?
💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1222442250)
Any reason to make this a global when it can be just a reference local in the body of `FUZZ_TARGET_INIT`?
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1222442250)
Any reason to make this a global when it can be just a reference local in the body of `FUZZ_TARGET_INIT`?
💬 jarolrod commented on pull request "ci: Nuke Android APK task, Use credits for tsan":
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1581881457)
@MarcoFalke withdrew NACK, I've misunderstood intentions and outcomes of this PR. It is true that right now outside of checking for build support, this doesn't do anything for this repo.
(https://github.com/bitcoin/bitcoin/pull/27834#issuecomment-1581881457)
@MarcoFalke withdrew NACK, I've misunderstood intentions and outcomes of this PR. It is true that right now outside of checking for build support, this doesn't do anything for this repo.
💬 MarcoFalke commented on pull request "kernel: Remove args, settings, chainparams, chainparamsbase from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27576#issuecomment-1581909843)
nit: In the last commit, any reason to use `\ ` and `\:`? Seems to pass with just ` ` and `:` for me.
lgtm ACK db77f87c6365cb5f414036d6bfb1a12705772028 🍄
<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/K
...
(https://github.com/bitcoin/bitcoin/pull/27576#issuecomment-1581909843)
nit: In the last commit, any reason to use `\ ` and `\:`? Seems to pass with just ` ` and `:` for me.
lgtm ACK db77f87c6365cb5f414036d6bfb1a12705772028 🍄
<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/K
...
👍 vasild approved a pull request: "p2p: return `CSubNet` in `LookupSubNet`"
(https://github.com/bitcoin/bitcoin/pull/26078#pullrequestreview-1469207921)
ACK fb3e812277041f239b97b88689a5076796d75b9b
(https://github.com/bitcoin/bitcoin/pull/26078#pullrequestreview-1469207921)
ACK fb3e812277041f239b97b88689a5076796d75b9b
💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1582072953)
`4c867de996...35fc849412`: address a suggestion
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1582072953)
`4c867de996...35fc849412`: address a suggestion