💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1665396151)
https://github.com/bitcoin/bitcoin/pull/28214 is the second step
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1665396151)
https://github.com/bitcoin/bitcoin/pull/28214 is the second step
💬 pinheadmz commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#discussion_r1284264509)
Ah 🤦♂️ the comment you added in response to my first review. Thanks!
(https://github.com/bitcoin/bitcoin/pull/28161#discussion_r1284264509)
Ah 🤦♂️ the comment you added in response to my first review. Thanks!
💬 MarcoFalke commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#discussion_r1284267996)
I think if one wants to not run the CI again, they'll need to use `rm` instead of `prune`, no? The goal of prune is to only remove dangling/stale/intermediate image layers?
(https://github.com/bitcoin/bitcoin/pull/27793#discussion_r1284267996)
I think if one wants to not run the CI again, they'll need to use `rm` instead of `prune`, no? The goal of prune is to only remove dangling/stale/intermediate image layers?
💬 stickies-v commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#discussion_r1284283773)
Yeah, would you prefer it like this?
````diff
diff --git a/ci/README.md b/ci/README.md
index ab5e6ce866..16310b7703 100644
--- a/ci/README.md
+++ b/ci/README.md
@@ -26,13 +26,19 @@ To run the test stage with a specific configuration,
FILE_ENV="./ci/test/00_setup_env_arm.sh" ./ci/test_run_all.sh
```
-The ci automatically prunes all dangling docker images every time it runs. If you want to perform
-the cleanup manually (e.g. if you won't be running the ci again), you can run
+The
...
(https://github.com/bitcoin/bitcoin/pull/27793#discussion_r1284283773)
Yeah, would you prefer it like this?
````diff
diff --git a/ci/README.md b/ci/README.md
index ab5e6ce866..16310b7703 100644
--- a/ci/README.md
+++ b/ci/README.md
@@ -26,13 +26,19 @@ To run the test stage with a specific configuration,
FILE_ENV="./ci/test/00_setup_env_arm.sh" ./ci/test_run_all.sh
```
-The ci automatically prunes all dangling docker images every time it runs. If you want to perform
-the cleanup manually (e.g. if you won't be running the ci again), you can run
+The
...
💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1665456319)
Updated b0beb4c504da29c27358d4602a045aaab39305f6 -> 9e80d0b754a28733c79a52c8e0431616c31d071c ([`pr/bresult2.38`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.38) -> [`pr/bresult2.39`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.39), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.38..pr/bresult2.39)) to fix `operator>>` compile error that seemed to happen with newer versions of clang: https://cirrus-ci.com/task/4622529512341504?logs=ci#L2370
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1665456319)
Updated b0beb4c504da29c27358d4602a045aaab39305f6 -> 9e80d0b754a28733c79a52c8e0431616c31d071c ([`pr/bresult2.38`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.38) -> [`pr/bresult2.39`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.39), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.38..pr/bresult2.39)) to fix `operator>>` compile error that seemed to happen with newer versions of clang: https://cirrus-ci.com/task/4622529512341504?logs=ci#L2370
💬 Retropex commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665460988)
Concept NACK.
We already have enough problems with inscriptions (That the developer still seems to ignore). In addition, why delete the `-datacarrier` option? This is an important option that gives nodes runner the choice, you could have simply removed the default limit.
Instead, focus on giving nodes runner the choice by implementing more options to customize the mempool policy.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665460988)
Concept NACK.
We already have enough problems with inscriptions (That the developer still seems to ignore). In addition, why delete the `-datacarrier` option? This is an important option that gives nodes runner the choice, you could have simply removed the default limit.
Instead, focus on giving nodes runner the choice by implementing more options to customize the mempool policy.
💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284321897)
Would it make sense to test the other edge, 1-of-0?
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284321897)
Would it make sense to test the other edge, 1-of-0?
📝 darosior opened a pull request: "fuzz: fix a couple incorrect assertions in the `coins_view` target"
(https://github.com/bitcoin/bitcoin/pull/28215)
I ran into this while introducing a new target with an in-memory `CCoinsViewDB` as the backend view which made the code code paths with those assertions actually reachable.
See commits messages for details.
(https://github.com/bitcoin/bitcoin/pull/28215)
I ran into this while introducing a new target with an in-memory `CCoinsViewDB` as the backend view which made the code code paths with those assertions actually reachable.
See commits messages for details.
👍 stickies-v approved a pull request: "scripted-diff: Specify Python major version explicitly on Windows"
(https://github.com/bitcoin/bitcoin/pull/28213#pullrequestreview-1562734864)
utACK 6a7686b44618eabd2f8ee9f1d357cfeb1bce6d91
(https://github.com/bitcoin/bitcoin/pull/28213#pullrequestreview-1562734864)
utACK 6a7686b44618eabd2f8ee9f1d357cfeb1bce6d91
💬 iBeizsley commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665503510)
> The 40 or 80B limit was based on storing a hash or two, surprisingly nobody thought at that time that you must store a signed hash as @ChristopherA and myself wants to do
Any hash you include in an op return is a signed hash. It's signed by the keys controlling the inputs.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665503510)
> The 40 or 80B limit was based on storing a hash or two, surprisingly nobody thought at that time that you must store a signed hash as @ChristopherA and myself wants to do
Any hash you include in an op return is a signed hash. It's signed by the keys controlling the inputs.
💬 MarcoFalke commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284353466)
What is the benefit? Looks like this is more code, easier to break (when for example a type width changes, or when a new field is added), as well as more wasteful (the early return is now removed and the fuzz engine will do a full run even if the fuzz input buffer is the empty string)?
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284353466)
What is the benefit? Looks like this is more code, easier to break (when for example a type width changes, or when a new field is added), as well as more wasteful (the early return is now removed and the fuzz engine will do a full run even if the fuzz input buffer is the empty string)?
💬 ChrisMartl commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665519819)
Concept NACK.
Bitcoin system faces the struggles of control taking; reduction of decentralization into an increment of centralization.
Some interested groups for that vision are striving to become “the” nodes of Bitcoin, uniting again mining and storage activities while wiping out the non-mining nodes via equipment and operation cost increment; one of that, is increasing the storage cost due to insertion of arbitrary data.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665519819)
Concept NACK.
Bitcoin system faces the struggles of control taking; reduction of decentralization into an increment of centralization.
Some interested groups for that vision are striving to become “the” nodes of Bitcoin, uniting again mining and storage activities while wiping out the non-mining nodes via equipment and operation cost increment; one of that, is increasing the storage cost due to insertion of arbitrary data.
💬 MarcoFalke commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1665525441)
Added some code to make the Windows CI pass
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1665525441)
Added some code to make the Windows CI pass
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284361515)
My thinking was that it would actually be more efficient to not rely on `ConsumeDeserializable` which needs to first guess the length of the byte vector to be consumed:
https://github.com/bitcoin/bitcoin/blob/a4ca4975880c4f870c6047065c70610af2529e74/src/test/fuzz/util.h#L95-L107
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284361515)
My thinking was that it would actually be more efficient to not rely on `ConsumeDeserializable` which needs to first guess the length of the byte vector to be consumed:
https://github.com/bitcoin/bitcoin/blob/a4ca4975880c4f870c6047065c70610af2529e74/src/test/fuzz/util.h#L95-L107
💬 MarcoFalke commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284364488)
Ok, that could be. Maybe a benchmark can be done to see if it helps or hurts?
In any case, if you keep it, my preference would be to use `decltype()` to derive the type of the fields and not hardcode them.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284364488)
Ok, that could be. Maybe a benchmark can be done to see if it helps or hurts?
In any case, if you keep it, my preference would be to use `decltype()` to derive the type of the fields and not hardcode them.
📝 darosior opened a pull request: "fuzz: a new target for the coins database"
(https://github.com/bitcoin/bitcoin/pull/28216)
Similarly to #28209, this introduces a fuzz target for `CCoinsViewDb` by using an in-memory LevelDB. We reuse the body of the existing fuzz target for `coins_view`.
This is based on #28215, which fixes a couple mistakes in the `coins_view` fuzz logic exposed by this new target.
(https://github.com/bitcoin/bitcoin/pull/28216)
Similarly to #28209, this introduces a fuzz target for `CCoinsViewDb` by using an in-memory LevelDB. We reuse the body of the existing fuzz target for `coins_view`.
This is based on #28215, which fixes a couple mistakes in the `coins_view` fuzz logic exposed by this new target.
💬 paplorinc commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284374826)
Slightly unrelated: are there any tests that check multisig with different keys rather than repeating the same one, to make it more representative of a real-world scenario?
I'm just getting familiar with the codebase, I'm not sure whether it's important or not.
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284374826)
Slightly unrelated: are there any tests that check multisig with different keys rather than repeating the same one, to make it more representative of a real-world scenario?
I'm just getting familiar with the codebase, I'm not sure whether it's important or not.
💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284376542)
+1
> My thinking was that it would actually be more efficient to not rely on ConsumeDeserializable which needs to first guess the length of the byte vector to be consumed
My initial thought was it facilitates especially for cases that `files_count`/`files_count` is closer to their max possible value.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1284376542)
+1
> My thinking was that it would actually be more efficient to not rely on ConsumeDeserializable which needs to first guess the length of the byte vector to be consumed
My initial thought was it facilitates especially for cases that `files_count`/`files_count` is closer to their max possible value.
💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1284382381)
My view is that anything that is different between v1 and v2 connections ought to be handled by the respective transport class.
In V2 there are multiple stages a connection goes through (pubkey, garbage+terminator, garbage authentication packet, version negotiation packet, and finally application data during which bitcoin P2P messages can be sent); this will be implemented using a finite state machine on sender and receiver side to control what state we are in. Only during the last phase (appli
...
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1284382381)
My view is that anything that is different between v1 and v2 connections ought to be handled by the respective transport class.
In V2 there are multiple stages a connection goes through (pubkey, garbage+terminator, garbage authentication packet, version negotiation packet, and finally application data during which bitcoin P2P messages can be sent); this will be implemented using a finite state machine on sender and receiver side to control what state we are in. Only during the last phase (appli
...
🚀 fanquake merged a pull request: "refactor: serialization simplifications"
(https://github.com/bitcoin/bitcoin/pull/28203)
(https://github.com/bitcoin/bitcoin/pull/28203)