💬 ajtowns commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296625023)
Is this only a named namespace because `MaybeArg` duplicates the function name in `RPCHelpMan`? I think either this should be an anonymous namespace (and use `::MaybeArg` to reference this func, or rename it), or `::MaybeArg` should be declared `static`.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296625023)
Is this only a named namespace because `MaybeArg` duplicates the function name in `RPCHelpMan`? I think either this should be an anonymous namespace (and use `::MaybeArg` to reference this func, or rename it), or `::MaybeArg` should be declared `static`.
💬 ajtowns commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296648899)
No need for the `function` wrapper -- `MaybeArg(void(&check)(const RPCArg&), ..)`
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296648899)
No need for the `function` wrapper -- `MaybeArg(void(&check)(const RPCArg&), ..)`
💬 ajtowns commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296651693)
Shouldn't this be `!required && !holds_alternative` ? Why use `MaybeArg(1)` instead of `Arg(1)` for a required arg?
(One reason: maybe we want to be able to change the `RPCHelpMan` definition for a call at runtime, eg based on `-deprecatedrpc` -- in that case if `deprecatedrpc` turns a required arg into an optional, being able to use `MaybeArg` might make the code simpler. But in that case `MaybeArg` should just always work, and not have a check, I think?)
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296651693)
Shouldn't this be `!required && !holds_alternative` ? Why use `MaybeArg(1)` instead of `Arg(1)` for a required arg?
(One reason: maybe we want to be able to change the `RPCHelpMan` definition for a call at runtime, eg based on `-deprecatedrpc` -- in that case if `deprecatedrpc` turns a required arg into an optional, being able to use `MaybeArg` might make the code simpler. But in that case `MaybeArg` should just always work, and not have a check, I think?)
💬 ajtowns commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1296721837)
What's the point of an `Assume(false)` that just falls through to an `assert(false)` ?
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1296721837)
What's the point of an `Assume(false)` that just falls through to an `assert(false)` ?
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296736312)
Style-wise, I don't like to shadow global-namespace functions by name. I think it is better if every function has a unique name. I'll change it to `static DetailMaybeArg` without a namespace.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296736312)
Style-wise, I don't like to shadow global-namespace functions by name. I think it is better if every function has a unique name. I'll change it to `static DetailMaybeArg` without a namespace.
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296736382)
I don't like the style of function pointers and I think `<void(const RPCArg&)> check` is easier to parse than `void(&check)(const RPCArg&)`. But I may change this in the next push, if there is one.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296736382)
I don't like the style of function pointers and I think `<void(const RPCArg&)> check` is easier to parse than `void(&check)(const RPCArg&)`. But I may change this in the next push, if there is one.
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296740115)
Ok, I'll remove the check
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296740115)
Ok, I'll remove the check
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296754116)
Ah I guess I can use `using CheckFn = void(const RPCArg&);` to make it as easy to parse.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296754116)
Ah I guess I can use `using CheckFn = void(const RPCArg&);` to make it as easy to parse.
💬 MarcoFalke commented on pull request "lint: fix custom mypy cache dir setting":
(https://github.com/bitcoin/bitcoin/pull/28184#discussion_r1296763658)
No, I mean `/test/`, not `./test/`.
With `/test` being next to `/bin`, etc. I don't think `.gitignore` will work on the literal root of your filesystem. `.gitignore` should be limited to the folder it is located in.
(https://github.com/bitcoin/bitcoin/pull/28184#discussion_r1296763658)
No, I mean `/test/`, not `./test/`.
With `/test` being next to `/bin`, etc. I don't think `.gitignore` will work on the literal root of your filesystem. `.gitignore` should be limited to the folder it is located in.
💬 owenstrevor commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681757825)
NACK
This discussion is a waste of time that could be better spent on talking solutions that improve Bitcoin, like Utreexo.
The proposed solution does not stop these transactions, but is like locking the front door of your house when the window is wide open and anyone can jump through it. It merely gives the illusion of accomplishment. It's theater.
Finally, these transactions will get priced out and go away if people actually use Bitcoin and the demand for blockspace reaches an actual
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681757825)
NACK
This discussion is a waste of time that could be better spent on talking solutions that improve Bitcoin, like Utreexo.
The proposed solution does not stop these transactions, but is like locking the front door of your house when the window is wide open and anyone can jump through it. It merely gives the illusion of accomplishment. It's theater.
Finally, these transactions will get priced out and go away if people actually use Bitcoin and the demand for blockspace reaches an actual
...
💬 MarcoFalke commented on pull request "test: display abrupt shutdown errors in console output":
(https://github.com/bitcoin/bitcoin/pull/28253#discussion_r1296776262)
> Yet, I'm a bit confused about where we currently stand in the discussion. Could you please clarify the proposed changes?
Not sure myself (I haven't tested this yet, just looked at the code). However, I think overall the best UX would be if to append the stderr to this message, see https://github.com/bitcoin/bitcoin/pull/28253/files#r1294258580 (using `*` and `\n` to format it nicely)
(https://github.com/bitcoin/bitcoin/pull/28253#discussion_r1296776262)
> Yet, I'm a bit confused about where we currently stand in the discussion. Could you please clarify the proposed changes?
Not sure myself (I haven't tested this yet, just looked at the code). However, I think overall the best UX would be if to append the stderr to this message, see https://github.com/bitcoin/bitcoin/pull/28253/files#r1294258580 (using `*` and `\n` to format it nicely)
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#issuecomment-1681764096)
Force pushed to allow `MaybeArg` in all contexts.
(https://github.com/bitcoin/bitcoin/pull/28230#issuecomment-1681764096)
Force pushed to allow `MaybeArg` in all contexts.
💬 ajtowns commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296805155)
static in a named namespace would be fine too :shrug:
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1296805155)
static in a named namespace would be fine too :shrug:
💬 RobinLinus commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681797275)
> The usable data storage capabilities of p2wsh are equal to p2pk with 32 bytes per outpoint.
You're assuming that they would use p2pk with _compressed_ keys. _Uncompressed_ keys are cheaper though because they can store 64 bytes.
> This would increase the number of required outpoints by 3x compared to p2ms.
The number of outpoints is irrelevant. Relevant is the per-byte cost of bloating the UTXO set.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681797275)
> The usable data storage capabilities of p2wsh are equal to p2pk with 32 bytes per outpoint.
You're assuming that they would use p2pk with _compressed_ keys. _Uncompressed_ keys are cheaper though because they can store 64 bytes.
> This would increase the number of required outpoints by 3x compared to p2ms.
The number of outpoints is irrelevant. Relevant is the per-byte cost of bloating the UTXO set.
💬 vasild commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296810743)
I mean, if we already have an inbound connection from `1.2.3.4` (ie we are connected to `1.2.3.4:somerandomport`) and this code checks whether we are connected to `1.2.3.4:8333` it would wrongly assume that we are not and would open an outbound connection to that address.
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296810743)
I mean, if we already have an inbound connection from `1.2.3.4` (ie we are connected to `1.2.3.4:somerandomport`) and this code checks whether we are connected to `1.2.3.4:8333` it would wrongly assume that we are not and would open an outbound connection to that address.
💬 russeree commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681805139)
> You're assuming that they would use p2pk with _compressed_ keys. _Uncompressed_ keys are cheaper though because they can store 64 bytes.
uncompressed pubkeys dont work that way since Bitcoin Core checks to make sure the Y is valid for X. As such trying to attach arb. data to X and Y will cause that check to fail and the script won't be considered standard. This is why stamps uses uncompressed public keys.
See my reply here.
https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681805139)
> You're assuming that they would use p2pk with _compressed_ keys. _Uncompressed_ keys are cheaper though because they can store 64 bytes.
uncompressed pubkeys dont work that way since Bitcoin Core checks to make sure the Y is valid for X. As such trying to attach arb. data to X and Y will cause that check to fail and the script won't be considered standard. This is why stamps uses uncompressed public keys.
See my reply here.
https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667
...
💬 RobinLinus commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681849095)
> multisig checks only check for valid size and not valid (x,y) components
Yes, that's what I had in mind.
> Not completely true, the more outpoints in the utxo set the longer the lookup time and the bigger the index gets also more disk write activity.
Still, the relevant metric is the cost of *"polluting the UTXO set"*. If a spammer is just trying to maximize the _number_ of UTXOs then the cheapest way is probably p2wkh.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681849095)
> multisig checks only check for valid size and not valid (x,y) components
Yes, that's what I had in mind.
> Not completely true, the more outpoints in the utxo set the longer the lookup time and the bigger the index gets also more disk write activity.
Still, the relevant metric is the cost of *"polluting the UTXO set"*. If a spammer is just trying to maximize the _number_ of UTXOs then the cheapest way is probably p2wkh.
💬 ajtowns commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1296854852)
For your consideration: https://github.com/ajtowns/bitcoin/commit/87c509c6d6ee0d355d08e0d4bc60bc01d4a0ad60 . I expanded the `WITH_LOCK` out in `GenerateWaitSockets` because I thought that was clearer than trying to make it an expression. Keeps the signature of `SocketSendData` the same, doesn't add any additional locking, and avoids the dummy `data_left` in `PushMessage`.
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1296854852)
For your consideration: https://github.com/ajtowns/bitcoin/commit/87c509c6d6ee0d355d08e0d4bc60bc01d4a0ad60 . I expanded the `WITH_LOCK` out in `GenerateWaitSockets` because I thought that was clearer than trying to make it an expression. Keeps the signature of `SocketSendData` the same, doesn't add any additional locking, and avoids the dummy `data_left` in `PushMessage`.
💬 darosior commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1296828571)
Yes or maybe have a new directory for everything that's "working with scripts" (`addresstype`, `descriptor`, `signingprovider`, ..).
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1296828571)
Yes or maybe have a new directory for everything that's "working with scripts" (`addresstype`, `descriptor`, `signingprovider`, ..).
🤔 darosior reviewed a pull request: "Break up script/standard.{h/cpp}"
(https://github.com/bitcoin/bitcoin/pull/28244#pullrequestreview-1582016616)
Code review ACK 91d924ede1b421df31c895f4f43359e453a09ca5.
However i think you can just drop the largest commit here (7a172c76d2361fc3cdf6345590e26c79a7821672) and save a (+291, -247). It would also make more sense IMO. You state:
> - `TaprootSpendData` and `TaprootBuilder` (and their utility functions and structs) are moved to SigningProvider as these are used only during signing.
> - `standard.{cpp/h}` is renamed to `solver.{cpp/h}` since that's all that's left in the file after the above
...
(https://github.com/bitcoin/bitcoin/pull/28244#pullrequestreview-1582016616)
Code review ACK 91d924ede1b421df31c895f4f43359e453a09ca5.
However i think you can just drop the largest commit here (7a172c76d2361fc3cdf6345590e26c79a7821672) and save a (+291, -247). It would also make more sense IMO. You state:
> - `TaprootSpendData` and `TaprootBuilder` (and their utility functions and structs) are moved to SigningProvider as these are used only during signing.
> - `standard.{cpp/h}` is renamed to `solver.{cpp/h}` since that's all that's left in the file after the above
...