💬 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)
💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284383877)
I think it's very related and also my concern, is this test as good as one with K different keys. I'm hoping @Sjors can provide some insight here. My understanding of the other tests in this file is limited because they are not documented, but they seem to be doing something similar to this. On the other hand I could of course do what the previous test did, to use the "H_POINT" approach and just throw one XPRV in there somewhere. If that makes the test more real-world like or improves the test i
...
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284383877)
I think it's very related and also my concern, is this test as good as one with K different keys. I'm hoping @Sjors can provide some insight here. My understanding of the other tests in this file is limited because they are not documented, but they seem to be doing something similar to this. On the other hand I could of course do what the previous test did, to use the "H_POINT" approach and just throw one XPRV in there somewhere. If that makes the test more real-world like or improves the test i
...
🚀 fanquake merged a pull request: "scripted-diff: Specify Python major version explicitly on Windows"
(https://github.com/bitcoin/bitcoin/pull/28213)
(https://github.com/bitcoin/bitcoin/pull/28213)
💬 Sun0fABeach commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665591754)
> Rational: there's lots of ways for people to publish data in the Bitcoin chain, and lots of data has been published. There's no reason for us to place artificial limits on this particular method.
There is plenty of reasons to place limits on **any** method that abuses the timechain as data storage. Any relaxation of the default settings validates existing spam and is unacceptable. Give node runners strong defaults and options to mitigate current spam strategies before doing any of these twe
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665591754)
> Rational: there's lots of ways for people to publish data in the Bitcoin chain, and lots of data has been published. There's no reason for us to place artificial limits on this particular method.
There is plenty of reasons to place limits on **any** method that abuses the timechain as data storage. Any relaxation of the default settings validates existing spam and is unacceptable. Give node runners strong defaults and options to mitigate current spam strategies before doing any of these twe
...
💬 luke-jr commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665602678)
> the idea is not to store a video on bitcoin but indeed a reference to it like bittorrent
You can already do that within the current datacarriersize. Removing the limit has no purpose other than gratuitious abuse.
>As different people explained this change is really needed and really necessary for the future of bitcoin
This is completely false and nonsensical. It does literally nothing good for Bitcoin, only harms Bitcoin.
> Regarding the 80 B number I don t understand where it com
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665602678)
> the idea is not to store a video on bitcoin but indeed a reference to it like bittorrent
You can already do that within the current datacarriersize. Removing the limit has no purpose other than gratuitious abuse.
>As different people explained this change is really needed and really necessary for the future of bitcoin
This is completely false and nonsensical. It does literally nothing good for Bitcoin, only harms Bitcoin.
> Regarding the 80 B number I don t understand where it com
...
💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1284431053)
I think that it worth. One of the fuzzing goals is to assert expected behavior externally. Black-box style. So if the internal implementation changes by mistake (or if there is a way to by-pass the expected behavior that we aren't aware of), the fuzzer catches it.
Also, the internal BnB `ComputeSetAndWaste` call checks that the final selection is within a certain range: `target <= value < target + change cost`. It is indirectly checking `GetChange()` result but I don't think that we should r
...
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1284431053)
I think that it worth. One of the fuzzing goals is to assert expected behavior externally. Black-box style. So if the internal implementation changes by mistake (or if there is a way to by-pass the expected behavior that we aren't aware of), the fuzzer catches it.
Also, the internal BnB `ComputeSetAndWaste` call checks that the final selection is within a certain range: `target <= value < target + change cost`. It is indirectly checking `GetChange()` result but I don't think that we should r
...
💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1284434687)
Fair enough, gonna address it.
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1284434687)
Fair enough, gonna address it.