Bitcoin Core Github
43 subscribers
122K links
Download Telegram
👍 brunoerg approved a pull request: "fuzz: Limit p2p fuzz targets to MAX_PROTOCOL_MESSAGE_LENGTH"
(https://github.com/bitcoin/bitcoin/pull/29079#pullrequestreview-1786739591)
crACK fa769d3e41daec696452b8a0a8753ba511b0a4b5
🤔 stickies-v reviewed a pull request: "Nuke adjusted time (attempt 2)"
(https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1786684919)
Looks like a nice improvement, but still improving my understanding so comments are limited to code review now, and not the concept. It would be helpful if at least some of the commits had a bit more rationale in the description.
💬 stickies-v commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430002494)
nit: ["AssertLockNotHeld() can be omitted if LOCK() is called unconditionally after it because LOCK() does the same check as AssertLockNotHeld() internally, for non-recursive mutexes"](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization)
💬 stickies-v commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1429975769)
1. I think the calculation is wrong: the average of the 2 middle elements should be taken in case of an even number of elements.
2. UB: `sorted_copy` being an array of size `N`, I think we'd be sorting uninitialized values if `m_index < N`?
3. nit: the scope of the lock can be reduced

Potential fix addressing all 3:

<details>
<summary>git diff on 8b1c133921</summary>

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 664992dcab..9ade704fe9 100644
--- a/src/
...
💬 stickies-v commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430024540)
const nit
```suggestion
const auto peerman_info{node.peerman->GetInfo()};
```
💬 stickies-v commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430056990)
Wdyt about making the offsets typesafe with `std::chrono::seconds`?

<details>
<summary>git diff on 8b1c133921</summary>

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 664992dcab..4a7243b82f 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -193,12 +193,12 @@ private:
static constexpr size_t N{199};

mutable Mutex m_mutex;
- std::array<int64_t, N> m_offsets GUARDED_BY(m_mutex){};
+ std::array<std::chrono::seconds, N>
...
💬 theDavidCoen commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860335728)
Concept ACK.

These kind of parameters should default to false.
Node operators should be active users and decide which feature they want to enable for their node.

In this specific case, permitbaremultisig should default to zero or nodes would add and broadcast unconventional multisig transactions by default, and include also public keys out of the Bitcoin spectrum (uncompressed keys that are not points on the secp256k1 curve), which cannot be other than spam.
Node operators should be able
...
💬 TheCharlatan commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860358206)
Concept ACK
💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430078828)
> I think the calculation is wrong: the average of the 2 middle elements should be taken in case of an even number of elements.

yes🤦

> UB: sorted_copy being an array of size N, I think we'd be sorting uninitialized values if m_index < N?

No, `m_offsets` is not uninitialized
💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430080537)
Good idea, but I'd prefer to leave that for a follow up
💬 fanquake commented on pull request "contrib: drop GCC MAX_VERSION to 4.3.0 in symbol-check":
(https://github.com/bitcoin/bitcoin/pull/28844#issuecomment-1860400750)
Guix Build (aarch64):
```bash
018360e32742367f372f48529ac321758dad7f64f42538807f06b2129e0d97fd guix-build-ff896d25819d/output/aarch64-linux-gnu/SHA256SUMS.part
8958317b7f79cb2839bed4f089c5c580be8dd3434563d2a797bb8f05905fbec5 guix-build-ff896d25819d/output/aarch64-linux-gnu/bitcoin-ff896d25819d-aarch64-linux-gnu-debug.tar.gz
71f0fd9abd4e354b87c4f670bdb17560f186f251d992f040c50239e3fbc783af guix-build-ff896d25819d/output/aarch64-linux-gnu/bitcoin-ff896d25819d-aarch64-linux-gnu.tar.gz
f82cf4
...
🚀 fanquake merged a pull request: "contrib: drop GCC MAX_VERSION to 4.3.0 in symbol-check"
(https://github.com/bitcoin/bitcoin/pull/28844)
🚀 fanquake merged a pull request: "fuzz: Limit p2p fuzz targets to MAX_PROTOCOL_MESSAGE_LENGTH"
(https://github.com/bitcoin/bitcoin/pull/29079)
🚀 fanquake merged a pull request: "build: Bump guix time-machine to unlock riscv64 metal"
(https://github.com/bitcoin/bitcoin/pull/29078)
fanquake closed an issue: "./contrib/guix/guix-build does not work on riscv64"
(https://github.com/bitcoin/bitcoin/issues/29020)
📝 SatoshiNT0 opened a pull request: "bitcoin"
(https://github.com/bitcoin/bitcoin/pull/29106)
<!--
*** 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
...
fanquake closed a pull request: "bitcoin"
(https://github.com/bitcoin/bitcoin/pull/29106)
📝 fanquake locked a pull request: "bitcoin"
(https://github.com/bitcoin/bitcoin/pull/29106)
<!--
*** 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
...
💬 fanquake commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1860456382)
If we are going to change it here, should also switch in `hash.cpp`:

https://github.com/bitcoin/bitcoin/blob/c840dea27edfcfc67beb756ca6eda27b319f93d5/src/hash.cpp#L12-L15

Also cc @theuni.
📝 kristapsk opened a pull request: "Fix spelling errors"
(https://github.com/bitcoin/bitcoin/pull/29107)
Found these when running lint tests locally.

```
src/rpc/util.h:405: falsy ==> falsely, false
src/rpc/util.h:408: falsy ==> falsely, false
src/test/fuzz/package_eval.cpp:214: non-existant ==> non-existent
src/test/span_tests.cpp:56: memeber ==> member
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```

Guess it's because I'm having different version of codespell?

In any case,
...