💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2228528366)
was chatting offline and was reminded of why I landed on a witness program over doing script trickery like:
`OP_DEPTH OP_NOT`
the issue here is that a miner could insert a `OP_NOP` onto the scriptSig, resulting in a different valid txid, breaking presigned transaction chains. Seems to suggest you really do need segwit here.
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2228528366)
was chatting offline and was reminded of why I landed on a witness program over doing script trickery like:
`OP_DEPTH OP_NOT`
the issue here is that a miner could insert a `OP_NOP` onto the scriptSig, resulting in a different valid txid, breaking presigned transaction chains. Seems to suggest you really do need segwit here.
💬 ryanofsky commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2228550103)
Code review ACK e67e4669a9e263d90f0e276c8e65e93c39170375, but I think the current implementation is too complicated and has too many layers of indirection.
Would suggest applying the following patch and squashing the PR down to a single commit which would be just +41/-16 with the suggested changes:
<details><summary>diff</summary>
<p>
```diff
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -313,7 +313,6 @@ BITCOIN_CORE_H = \
util/hasher.h \
util/insert.h \
util/macros.h \
...
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2228550103)
Code review ACK e67e4669a9e263d90f0e276c8e65e93c39170375, but I think the current implementation is too complicated and has too many layers of indirection.
Would suggest applying the following patch and squashing the PR down to a single commit which would be just +41/-16 with the suggested changes:
<details><summary>diff</summary>
<p>
```diff
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -313,7 +313,6 @@ BITCOIN_CORE_H = \
util/hasher.h \
util/insert.h \
util/macros.h \
...
💬 maflcko commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677866036)
> if the test is run in a loop, sometimes `"socket no message in first %I seconds"` is displayed and sometimes `"V2 handshake timeout"` is displayed.
I couldn't reproduce this locally. Also, it seems odd that one member field of a peer affects the field of another peer.
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677866036)
> if the test is run in a loop, sometimes `"socket no message in first %I seconds"` is displayed and sometimes `"V2 handshake timeout"` is displayed.
I couldn't reproduce this locally. Also, it seems odd that one member field of a peer affects the field of another peer.
💬 stratospher commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677876281)
here's a failure I get when removing `peer2`:
```
$ test/functional/p2p_v2_misbehaving.py
2024-07-15T13:53:35.169000Z TestFramework (INFO): PRNG seed is: 5439305671400460804
2024-07-15T13:53:35.169000Z TestFramework (INFO): Initializing test directory /var/folders/23/1bj9v8kx4pjdz4nm49dhx2440000gn/T/bitcoin_func_test_72i8znef
2024-07-15T13:53:35.444000Z TestFramework (INFO): Sending ellswift bytes in parts to ensure that response from responder is received only when
2024-07-15T13:53:35.
...
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677876281)
here's a failure I get when removing `peer2`:
```
$ test/functional/p2p_v2_misbehaving.py
2024-07-15T13:53:35.169000Z TestFramework (INFO): PRNG seed is: 5439305671400460804
2024-07-15T13:53:35.169000Z TestFramework (INFO): Initializing test directory /var/folders/23/1bj9v8kx4pjdz4nm49dhx2440000gn/T/bitcoin_func_test_72i8znef
2024-07-15T13:53:35.444000Z TestFramework (INFO): Sending ellswift bytes in parts to ensure that response from responder is received only when
2024-07-15T13:53:35.
...
💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-2228574679)
Rebased
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-2228574679)
Rebased
💬 hebasto commented on pull request "Enable customisation of QR Code font":
(https://github.com/bitcoin-core/gui/pull/820#issuecomment-2228582318)
Concept ACK on adding a new customization option for the QR code font.
Lightly tested b14c9d0572eeae0fa286e0d2b96474a6f587b18e. It seems to work as expected.
However, I think that QR image widget must consider the font size for adjusting its own size. Otherwise, the image can be unreadable and confusing for the user:
The second commit modifies the code introduced in the first one. Therefore, pleas
...
(https://github.com/bitcoin-core/gui/pull/820#issuecomment-2228582318)
Concept ACK on adding a new customization option for the QR code font.
Lightly tested b14c9d0572eeae0fa286e0d2b96474a6f587b18e. It seems to work as expected.
However, I think that QR image widget must consider the font size for adjusting its own size. Otherwise, the image can be unreadable and confusing for the user:
The second commit modifies the code introduced in the first one. Therefore, pleas
...
💬 stratospher commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677885509)
>Also, it seems odd that one member field of a peer affects the field of another peer.
no, it doesn't! I just want some way to wait some time so that `sendSet` for the same peer can get set. it will eventually get set but not immediately.
we could replace:
```
peer2 = node0.add_p2p_connection(P2PInterface())
assert peer2.is_connected
```
with
```
import time; time.sleep(3)
```
and it would do!
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677885509)
>Also, it seems odd that one member field of a peer affects the field of another peer.
no, it doesn't! I just want some way to wait some time so that `sendSet` for the same peer can get set. it will eventually get set but not immediately.
we could replace:
```
peer2 = node0.add_p2p_connection(P2PInterface())
assert peer2.is_connected
```
with
```
import time; time.sleep(3)
```
and it would do!
💬 hebasto commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2228600085)
> Currently a user can open unlimited tx details dialogs for the same tx id.
Concept ACK. Such a behavior can confuses the user.
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2228600085)
> Currently a user can open unlimited tx details dialogs for the same tx id.
Concept ACK. Such a behavior can confuses the user.
💬 maflcko commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677898327)
Ok, I see. Would an alternative be to wait until the RPC's peer info `bytessent` or `bytesrecv` increased after the `send_raw_message`?
Alternatively, I presume the reason that `sendSet` is set is because `send_raw_message` is executed. So the comment could simply say the this is to ensure the message sent is actually received.
Anything is fine tough. I see now why it is needed.
(https://github.com/bitcoin/bitcoin/pull/30420#discussion_r1677898327)
Ok, I see. Would an alternative be to wait until the RPC's peer info `bytessent` or `bytesrecv` increased after the `send_raw_message`?
Alternatively, I presume the reason that `sendSet` is set is because `send_raw_message` is executed. So the comment could simply say the this is to ensure the message sent is actually received.
Anything is fine tough. I see now why it is needed.
💬 maflcko commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#issuecomment-2228609082)
ACK 5a6bb243adb705ac876ef1efd58a4768fa4ed033
(https://github.com/bitcoin/bitcoin/pull/30420#issuecomment-2228609082)
ACK 5a6bb243adb705ac876ef1efd58a4768fa4ed033
💬 hebasto commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2228659482)
> Tested successfully on Ubuntu 22.04.4 LTS. Only one dialogue is created per transaction and the clicked transaction is brought to the foreground
Tested on Ubuntu 24.04. It works with `QT_QPA_PLATFORM=xcb`, but fails with `QT_QPA_PLATFORM=wayland`.
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2228659482)
> Tested successfully on Ubuntu 22.04.4 LTS. Only one dialogue is created per transaction and the clicked transaction is brought to the foreground
Tested on Ubuntu 24.04. It works with `QT_QPA_PLATFORM=xcb`, but fails with `QT_QPA_PLATFORM=wayland`.
💬 hebasto commented on pull request "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection":
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2228675653)
@pablomartin4btc Did you consider https://github.com/bitcoinknots/bitcoin/issues/83?
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2228675653)
@pablomartin4btc Did you consider https://github.com/bitcoinknots/bitcoin/issues/83?
🤔 furszy reviewed a pull request: "init: change shutdown order of load block thread and scheduler"
(https://github.com/bitcoin/bitcoin/pull/30435#pullrequestreview-2177924099)
> > It would help us catch these type of errors (if we still have them).
>
> Do you mean throwing an assert instead of blocking indefinitely? That might be more convenient than hanging indefinitely, but I'm not sure if this really makes much of a difference in practice, because both ways should be easily recognizable both by users and tests.
Yes, check if the scheduler is processing events and if not: log something + throw an error. I'm unsure regular users can detect and report which thre
...
(https://github.com/bitcoin/bitcoin/pull/30435#pullrequestreview-2177924099)
> > It would help us catch these type of errors (if we still have them).
>
> Do you mean throwing an assert instead of blocking indefinitely? That might be more convenient than hanging indefinitely, but I'm not sure if this really makes much of a difference in practice, because both ways should be easily recognizable both by users and tests.
Yes, check if the scheduler is processing events and if not: log something + throw an error. I'm unsure regular users can detect and report which thre
...
💬 ismaelsadeeq commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1677942835)
Indicate the assumed upper limits of this options?
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1677942835)
Indicate the assumed upper limits of this options?
💬 ismaelsadeeq commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1677950701)
Why are we allowing for this large values?
shouldnt this be
```suggestion
Assume(coinbase_max_additional_weight <= MAX_STANDARD_TX_WEIGHT);
options.coinbase_max_additional_weight = coinbase_max_additional_weight;
Assume(options.coinbase_output_max_additional_sigops <= MAX_STANDARD_TX_SIGOPS_COST);
```
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1677950701)
Why are we allowing for this large values?
shouldnt this be
```suggestion
Assume(coinbase_max_additional_weight <= MAX_STANDARD_TX_WEIGHT);
options.coinbase_max_additional_weight = coinbase_max_additional_weight;
Assume(options.coinbase_output_max_additional_sigops <= MAX_STANDARD_TX_SIGOPS_COST);
```
💬 ismaelsadeeq commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1677885951)
I think it should not be necessary to reference pool in here
```suggestion
/**
* The maximum additional weight that can be added to the coinbase
* scriptSig, witness and outputs. This must include any additional
* weight needed for larger CompactSize encoded lengths.
*/
size_t coinbase_max_additional_weight{4000};
/**
* The maximum additional sigops that can be added in coinbase
* transaction outputs.
...
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1677885951)
I think it should not be necessary to reference pool in here
```suggestion
/**
* The maximum additional weight that can be added to the coinbase
* scriptSig, witness and outputs. This must include any additional
* weight needed for larger CompactSize encoded lengths.
*/
size_t coinbase_max_additional_weight{4000};
/**
* The maximum additional sigops that can be added in coinbase
* transaction outputs.
...
💬 pablomartin4btc commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2228688682)
> Tested on Ubuntu 24.04. It works with `QT_QPA_PLATFORM=xcb`, but fails with `QT_QPA_PLATFORM=wayland`.
I'll take a look, thanks!
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2228688682)
> Tested on Ubuntu 24.04. It works with `QT_QPA_PLATFORM=xcb`, but fails with `QT_QPA_PLATFORM=wayland`.
I'll take a look, thanks!
💬 pablomartin4btc commented on pull request "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection":
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2228709837)
> @pablomartin4btc Did you consider [bitcoinknots/bitcoin#83](https://github.com/bitcoinknots/bitcoin/issues/83)?
Yeah, in fact the "switch to Overview" tab when changing/ selecting wallets, was introduced in #718, I can fix it here in a 2nd. commit which will do soon.
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2228709837)
> @pablomartin4btc Did you consider [bitcoinknots/bitcoin#83](https://github.com/bitcoinknots/bitcoin/issues/83)?
Yeah, in fact the "switch to Overview" tab when changing/ selecting wallets, was introduced in #718, I can fix it here in a 2nd. commit which will do soon.
💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-2228726277)
CI failure is unrelated.
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-2228726277)
CI failure is unrelated.
🤔 pablomartin4btc reviewed a pull request: "fix: rendering an amp characters in the wallet name for QMenu"
(https://github.com/bitcoin-core/gui/pull/828#pullrequestreview-2177986863)
Concept ACK - I'll test it soon.
There's a typo in the commit body ("missleading"), since you are there, perhaps a better prefix for its title could be "`gui:`" and a <ins>nit</ins> on the text could be:
```
The comment in the code regarding the use of an "&"
on a menu item is misleading. If a wallet name has an "&" in it,
it is not supposed to be interpreted as a hot-key, but it should be
shown as it is without replacing it to an underscore.
```
(https://github.com/bitcoin-core/gui/pull/828#pullrequestreview-2177986863)
Concept ACK - I'll test it soon.
There's a typo in the commit body ("missleading"), since you are there, perhaps a better prefix for its title could be "`gui:`" and a <ins>nit</ins> on the text could be:
```
The comment in the code regarding the use of an "&"
on a menu item is misleading. If a wallet name has an "&" in it,
it is not supposed to be interpreted as a hot-key, but it should be
shown as it is without replacing it to an underscore.
```