💬 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
...
💬 MarcoFalke commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1681873254)
rebased
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1681873254)
rebased
💬 MarcoFalke commented on pull request "ci: Add test-each-commit task":
(https://github.com/bitcoin/bitcoin/pull/28279#issuecomment-1681881324)
Will squash if there is conceptual agreement (Not sure when this concept last came up, but I think @dergoegge and @achow101 mentioned it?). Review question: Looks like the worst-case runtime is 1 hour per commit, so with GHA at most 6 commits could be tested before a timeout. Maybe a self-hosted worker with a longer timeout would be better?
(https://github.com/bitcoin/bitcoin/pull/28279#issuecomment-1681881324)
Will squash if there is conceptual agreement (Not sure when this concept last came up, but I think @dergoegge and @achow101 mentioned it?). Review question: Looks like the worst-case runtime is 1 hour per commit, so with GHA at most 6 commits could be tested before a timeout. Maybe a self-hosted worker with a longer timeout would be better?
💬 dergoegge commented on pull request "ci: Add test-each-commit task":
(https://github.com/bitcoin/bitcoin/pull/28279#issuecomment-1681892651)
Concept ACK
> Maybe a self-hosted worker with a longer timeout would be better?
Probably but I am wondering a bit if these task could end up clogging our self-hosted worker queue and hinder other jobs from running in their regular time?
(https://github.com/bitcoin/bitcoin/pull/28279#issuecomment-1681892651)
Concept ACK
> Maybe a self-hosted worker with a longer timeout would be better?
Probably but I am wondering a bit if these task could end up clogging our self-hosted worker queue and hinder other jobs from running in their regular time?
🤔 vasild reviewed a pull request: "Improves addnode / m_added_nodes logic"
(https://github.com/bitcoin/bitcoin/pull/28155#pullrequestreview-1582006631)
Approach ACK a6fcf17f9dde56637bcfe824bb3def83e764b38c
(https://github.com/bitcoin/bitcoin/pull/28155#pullrequestreview-1582006631)
Approach ACK a6fcf17f9dde56637bcfe824bb3def83e764b38c
💬 vasild commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296841654)
In the commit message of 12e601c33e `rpc: Prevents adding the same ip more than once when formatted differently`:
> ... formatting IPs in aborts
different,
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296841654)
In the commit message of 12e601c33e `rpc: Prevents adding the same ip more than once when formatted differently`:
> ... formatting IPs in aborts
different,
💬 vasild commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296821862)
nit, style (`const` and attach the `&` to `auto`):
```suggestion
for (const auto& r : resolved) {
```
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296821862)
nit, style (`const` and attach the `&` to `auto`):
```suggestion
for (const auto& r : resolved) {
```
💬 vasild commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296827360)
This log can be improved to something like this:
```
Not opening a new connection to pool.bitcoinnodes.org, already connected to 1.2.3.4
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296827360)
This log can be improved to something like this:
```
Not opening a new connection to pool.bitcoinnodes.org, already connected to 1.2.3.4
💬 vasild commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296882332)
nit: use `resolved.IsValid()` instead of `resolved != CService{}`.
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296882332)
nit: use `resolved.IsValid()` instead of `resolved != CService{}`.