📝 aureleoules converted_to_draft a pull request: "rpc: Optimize serialization disk space of dumptxoutset"
(https://github.com/bitcoin/bitcoin/pull/26045)
This is an attempt to implement #25675.
I was able to reduce the serialized utxo set from 5GB to 4.1GB on mainnet.
Closes #25675.
(https://github.com/bitcoin/bitcoin/pull/26045)
This is an attempt to implement #25675.
I was able to reduce the serialized utxo set from 5GB to 4.1GB on mainnet.
Closes #25675.
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1860108282)
Integrated changes from https://github.com/Sjors/bitcoin/pull/26: further noise simplification, and chunking large messages. This requires https://github.com/Fi3/stratum-1/tree/UpdateNoise until it's merged into the dev branch. I also renamed `SendMsg` in `noise.h` to `WriteMsg`. And rebased.
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1860108282)
Integrated changes from https://github.com/Sjors/bitcoin/pull/26: further noise simplification, and chunking large messages. This requires https://github.com/Fi3/stratum-1/tree/UpdateNoise until it's merged into the dev branch. I also renamed `SendMsg` in `noise.h` to `WriteMsg`. And rebased.
💬 maflcko commented on pull request "doc: Rework guix docs after 1.4 release":
(https://github.com/bitcoin/bitcoin/pull/28962#discussion_r1429911345)
thx, removed
(https://github.com/bitcoin/bitcoin/pull/28962#discussion_r1429911345)
thx, removed
💬 maflcko commented on pull request "doc: Rework guix docs after 1.4 release":
(https://github.com/bitcoin/bitcoin/pull/28962#discussion_r1429911489)
thx, done
(https://github.com/bitcoin/bitcoin/pull/28962#discussion_r1429911489)
thx, done
💬 maflcko commented on pull request "Update doc/policy/README.md":
(https://github.com/bitcoin/bitcoin/pull/29095#issuecomment-1860150991)
So I guess it would be better to update the docs to refer to that, than to start listing more options here.
(https://github.com/bitcoin/bitcoin/pull/29095#issuecomment-1860150991)
So I guess it would be better to update the docs to refer to that, than to start listing more options here.
💬 aureleoules commented on pull request "rpc: Optimize serialization disk space of dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1860158880)
Rebased
I had to slightly change the tests in `feature_assumeutxo.py` because I changed the encoding format of the dump. I added 2 bytes to the offset because of the new `size` (2 bytes) field.
(https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1860158880)
Rebased
I had to slightly change the tests in `feature_assumeutxo.py` because I changed the encoding format of the dump. I added 2 bytes to the offset because of the new `size` (2 bytes) field.
👋 aureleoules's pull request is ready for review: "rpc: Optimize serialization disk space of dumptxoutset"
(https://github.com/bitcoin/bitcoin/pull/26045)
(https://github.com/bitcoin/bitcoin/pull/26045)
💬 fanquake commented on issue "Performance decrease after tapscript miniscript":
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-1860166405)
cc @darosior @sipa
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-1860166405)
cc @darosior @sipa
💬 maflcko commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1860181524)
> I think you need an helper
Looks like you are right. I thought that it was possible to write a concept like
```cpp
template <class T>
concept ArrayLike = requires(T a) {
std::array<decltype(*a.begin()), a.size()>{};
};
```
But that doesn't work, because `a.size()` is unevaluated (https://eel.is/c++draft/expr.prim#req.general-2) and presumably template arguments must be evaluated?
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1860181524)
> I think you need an helper
Looks like you are right. I thought that it was possible to write a concept like
```cpp
template <class T>
concept ArrayLike = requires(T a) {
std::array<decltype(*a.begin()), a.size()>{};
};
```
But that doesn't work, because `a.size()` is unevaluated (https://eel.is/c++draft/expr.prim#req.general-2) and presumably template arguments must be evaluated?
💬 ajtowns commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1860216234)
> > I think you need an helper
>
> Looks like you are right. I thought that it was possible to write a concept like
>
> ```c++
> template <class T>
> concept ArrayLike = requires(T a) {
> std::array<decltype(*a.begin()), a.size()>{};
> };
> ```
>
> But that doesn't work, because `a.size()` is unevaluated (https://eel.is/c++draft/expr.prim#req.general-2) and presumably template arguments must be evaluated?
`ArrayLike` sounds like `ranges::contiguous_range` ? https://en.cppref
...
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1860216234)
> > I think you need an helper
>
> Looks like you are right. I thought that it was possible to write a concept like
>
> ```c++
> template <class T>
> concept ArrayLike = requires(T a) {
> std::array<decltype(*a.begin()), a.size()>{};
> };
> ```
>
> But that doesn't work, because `a.size()` is unevaluated (https://eel.is/c++draft/expr.prim#req.general-2) and presumably template arguments must be evaluated?
`ArrayLike` sounds like `ranges::contiguous_range` ? https://en.cppref
...
💬 darosior commented on issue "Performance decrease after tapscript miniscript":
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-1860221553)
Without profiling my hunch is that this performance decrease comes from using a less efficient but more optimal algorithm for satisfying a `multi_a` fragment:
https://github.com/bitcoin/bitcoin/blob/3695ecbf680a66b718f97d504308578d001eec49/src/script/miniscript.h#L1178-L1205
The [previously algorithm](https://github.com/bitcoin/bitcoin/commit/4f473ea515bc77b9138323dab8a741c063d32e8f#diff-8a974828ccf5a554c068f5e859e62d1ab1e5010c66baaa6d6b83f42b26b219adL286-L299) would use the first `k` availa
...
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-1860221553)
Without profiling my hunch is that this performance decrease comes from using a less efficient but more optimal algorithm for satisfying a `multi_a` fragment:
https://github.com/bitcoin/bitcoin/blob/3695ecbf680a66b718f97d504308578d001eec49/src/script/miniscript.h#L1178-L1205
The [previously algorithm](https://github.com/bitcoin/bitcoin/commit/4f473ea515bc77b9138323dab8a741c063d32e8f#diff-8a974828ccf5a554c068f5e859e62d1ab1e5010c66baaa6d6b83f42b26b219adL286-L299) would use the first `k` availa
...
💬 fanquake commented on pull request "build: Bump guix time-machine to unlock riscv64 metal":
(https://github.com/bitcoin/bitcoin/pull/29078#issuecomment-1860266173)
> Is there an easy way to print all affected/changed/bumped dependencies in the Bitcoin Core build graph?
A quick check is to compare the manifests of master vs the change, i.e:
```bash
diff guix-build-3695ecbf680a/var/profiles/x86_64-linux-gnu/manifest guix-build-fa87a2072b91/var/profiles/x86_64-linux-gnu/manifest
60c60
< "0.67"
---
> "0.68"
62c62
< "/gnu/store/4ck57lp0wdcslrc106c649yfm3fclmsl-moreutils-0.67")
---
> "/gnu/store/09jzmmg2h8cxf75imk0r1c3j0cx20
...
(https://github.com/bitcoin/bitcoin/pull/29078#issuecomment-1860266173)
> Is there an easy way to print all affected/changed/bumped dependencies in the Bitcoin Core build graph?
A quick check is to compare the manifests of master vs the change, i.e:
```bash
diff guix-build-3695ecbf680a/var/profiles/x86_64-linux-gnu/manifest guix-build-fa87a2072b91/var/profiles/x86_64-linux-gnu/manifest
60c60
< "0.67"
---
> "0.68"
62c62
< "/gnu/store/4ck57lp0wdcslrc106c649yfm3fclmsl-moreutils-0.67")
---
> "/gnu/store/09jzmmg2h8cxf75imk0r1c3j0cx20
...
👍 fanquake approved a pull request: "build: Bump guix time-machine to unlock riscv64 metal"
(https://github.com/bitcoin/bitcoin/pull/29078#pullrequestreview-1786733471)
ACK fa87a2072b91c591d2714bc70488b395c22df95d
Guix build (aarch64):
```bash
d137d8dc94b3d734965b7e50fac273f704e122f969430e1b43bfe21c9af31bbe guix-build-fa87a2072b91/output/aarch64-linux-gnu/SHA256SUMS.part
42d66d0daf6067a74c3ba4e55c012712ae5cca02c9123ed3ef7fd9a8f09c16d4 guix-build-fa87a2072b91/output/aarch64-linux-gnu/bitcoin-fa87a2072b91-aarch64-linux-gnu-debug.tar.gz
3ccda6af99c1dd4b54533d073dca82d5f252759cede55a335220065dfe24ee7c guix-build-fa87a2072b91/output/aarch64-linux-gnu/bitco
...
(https://github.com/bitcoin/bitcoin/pull/29078#pullrequestreview-1786733471)
ACK fa87a2072b91c591d2714bc70488b395c22df95d
Guix build (aarch64):
```bash
d137d8dc94b3d734965b7e50fac273f704e122f969430e1b43bfe21c9af31bbe guix-build-fa87a2072b91/output/aarch64-linux-gnu/SHA256SUMS.part
42d66d0daf6067a74c3ba4e55c012712ae5cca02c9123ed3ef7fd9a8f09c16d4 guix-build-fa87a2072b91/output/aarch64-linux-gnu/bitcoin-fa87a2072b91-aarch64-linux-gnu-debug.tar.gz
3ccda6af99c1dd4b54533d073dca82d5f252759cede55a335220065dfe24ee7c guix-build-fa87a2072b91/output/aarch64-linux-gnu/bitco
...
👍 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
(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.
(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)
(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/
...
(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()};
```
(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>
...
(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
...
(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
...