💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218088888)
Yes, exactly. Before this change the test sent the transaction to the node using RPC and so it becomes locally-submitted, sensitive, unbroadcast, our own, etc transaction. With this change to the test it sends the transaction to the node via the P2P interface, so it is treated as normal, foreign, non-sensitive transaction and thus included in the MEMPOOL reply.
Btw, if changes from c70da509e4224f738fa0229e1f434a854629acf2 `net_processing: omit unbroadcast txs from replies to GETDATA and MEMPO
...
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218088888)
Yes, exactly. Before this change the test sent the transaction to the node using RPC and so it becomes locally-submitted, sensitive, unbroadcast, our own, etc transaction. With this change to the test it sends the transaction to the node via the P2P interface, so it is treated as normal, foreign, non-sensitive transaction and thus included in the MEMPOOL reply.
Btw, if changes from c70da509e4224f738fa0229e1f434a854629acf2 `net_processing: omit unbroadcast txs from replies to GETDATA and MEMPO
...
💬 instagibbs commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1576826080)
would like to see an updated OP that details what changes have been made, including precisely how often these connections are being made
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1576826080)
would like to see an updated OP that details what changes have been made, including precisely how often these connections are being made
💬 hebasto commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1576836334)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1576836334)
Concept ACK.
💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1576840323)
On Sun, Jun 04, 2023 at 06:47:08PM -0700, Antoine Riard wrote:
> > To be clear, 0conf users have to "upgrade" every single time mempool policies/conditions change for any non-trivial amount of hashing power. Pretty much any mempool policy change can be exploited to double-spend unconfirmed transactions. That's why the only entities with any hope of relying on them are large, centralized, transaction processors with significant engineering resources. The nVersion=3 proposal is not special in this
...
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1576840323)
On Sun, Jun 04, 2023 at 06:47:08PM -0700, Antoine Riard wrote:
> > To be clear, 0conf users have to "upgrade" every single time mempool policies/conditions change for any non-trivial amount of hashing power. Pretty much any mempool policy change can be exploited to double-spend unconfirmed transactions. That's why the only entities with any hope of relying on them are large, centralized, transaction processors with significant engineering resources. The nVersion=3 proposal is not special in this
...
💬 Sjors commented on pull request "validation: log which peer sent us a header":
(https://github.com/bitcoin/bitcoin/pull/27826#discussion_r1218113430)
Since it's possible for `ProcessNewBlockHeaders` to return false without us returning, I wonder if we should only log if it returned `true` (as we do above).
(https://github.com/bitcoin/bitcoin/pull/27826#discussion_r1218113430)
Since it's possible for `ProcessNewBlockHeaders` to return false without us returning, I wonder if we should only log if it returned `true` (as we do above).
💬 brunoerg commented on issue "ignored getblocktxn takes longer to detect than is possible":
(https://github.com/bitcoin/bitcoin/issues/22295#issuecomment-1576863888)
Perhaps #27626 fixed it?
(https://github.com/bitcoin/bitcoin/issues/22295#issuecomment-1576863888)
Perhaps #27626 fixed it?
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1576875849)
> In case my node doesn't accept inbound connections and supports clearnet + tor, however, my addrman has majority ipv4/6 addresses, seems like there is a chance of not having any connection with a Tor peer, so it would make harder to know that my transaction has been propagated?
@brunoerg, Why? I couldn't follow. But anyway - as of now this PR does not wait for more than one peer to give back the transaction to us.
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1576875849)
> In case my node doesn't accept inbound connections and supports clearnet + tor, however, my addrman has majority ipv4/6 addresses, seems like there is a chance of not having any connection with a Tor peer, so it would make harder to know that my transaction has been propagated?
@brunoerg, Why? I couldn't follow. But anyway - as of now this PR does not wait for more than one peer to give back the transaction to us.
💬 willcl-ark commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1576885847)
Well, they would still be 32 bit values, just hex-encoded? But I take the point. I personally think prefixing hex with `0x` would be kind of horrible, as this would cascade into including pubkeys for example.
Is prefixing decimals with `0d` the best approach then?
I am still unclear on how much impact this would have downstream, and therefore whether any of these discussed changes would be accepted in any form. It seems like this could break much of the little scripting tooling we have av
...
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1576885847)
Well, they would still be 32 bit values, just hex-encoded? But I take the point. I personally think prefixing hex with `0x` would be kind of horrible, as this would cascade into including pubkeys for example.
Is prefixing decimals with `0d` the best approach then?
I am still unclear on how much impact this would have downstream, and therefore whether any of these discussed changes would be accepted in any form. It seems like this could break much of the little scripting tooling we have av
...
💬 instagibbs commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1218144248)
IIUC
total number of inflights doesn't appear to be changed here, it's still `MAX_PEER_TX_ANNOUNCEMENTS`
If the protected buckets are taken by attackers, an honest party can still insert an orphan into the unprotected bucket(under the MAX_PEER_TX_ANNOUNCEMENTS limit), and the whole package including the child should get fetched after a short delay.
So from a time-sensitive contract perspective, things should be strictly better than today?
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1218144248)
IIUC
total number of inflights doesn't appear to be changed here, it's still `MAX_PEER_TX_ANNOUNCEMENTS`
If the protected buckets are taken by attackers, an honest party can still insert an orphan into the unprotected bucket(under the MAX_PEER_TX_ANNOUNCEMENTS limit), and the whole package including the child should get fetched after a short delay.
So from a time-sensitive contract perspective, things should be strictly better than today?
💬 willcl-ark commented on issue "Sync does not continue after reindex until a block is inved":
(https://github.com/bitcoin/bitcoin/issues/8239#issuecomment-1576902123)
I also seem unable to reproduce this behaviour on master at 7f2019755d, but I didn't check on mainnet with a long chain (and high chain work as suggested by Marco), instead I stopped a regtest node (a) in GDB while it was reindexing and generated new blocks on a second already-connected node (b), before continuing with the reindex.
The headers were received on node (a) before the reindex had completed, which then sent `getheaders` and synced to the new tip, without recieving any further block
...
(https://github.com/bitcoin/bitcoin/issues/8239#issuecomment-1576902123)
I also seem unable to reproduce this behaviour on master at 7f2019755d, but I didn't check on mainnet with a long chain (and high chain work as suggested by Marco), instead I stopped a regtest node (a) in GDB while it was reindexing and generated new blocks on a second already-connected node (b), before continuing with the reindex.
The headers were received on node (a) before the reindex had completed, which then sent `getheaders` and synced to the new tip, without recieving any further block
...
👍 TheCharlatan approved a pull request: "guix: remove cURL from build env"
(https://github.com/bitcoin/bitcoin/pull/27779#pullrequestreview-1462683737)
ACK 641897a83dc9d40b618efbae67c3beb90a1f34f8
Guix build:
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
7e9b3758aadd87db9c89fec5c05ca7a5020aaebf185daee4f7570dd1eeb23355 guix-build-641897a83dc9/output/aarch64-linux-gnu/SHA256SUMS.part
494f06d1f21ad20aa8cc8f61c4dfeda053215f41bb2aa0bfd47343909edf4dad guix-build-641897a83dc9/output/aarch64-linux-gnu/bitcoin-641897a83dc9-aarch64-linux-gnu-debug.tar.gz
8a3912a0ff0cb41
...
(https://github.com/bitcoin/bitcoin/pull/27779#pullrequestreview-1462683737)
ACK 641897a83dc9d40b618efbae67c3beb90a1f34f8
Guix build:
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
7e9b3758aadd87db9c89fec5c05ca7a5020aaebf185daee4f7570dd1eeb23355 guix-build-641897a83dc9/output/aarch64-linux-gnu/SHA256SUMS.part
494f06d1f21ad20aa8cc8f61c4dfeda053215f41bb2aa0bfd47343909edf4dad guix-build-641897a83dc9/output/aarch64-linux-gnu/bitcoin-641897a83dc9-aarch64-linux-gnu-debug.tar.gz
8a3912a0ff0cb41
...
💬 mzumsande commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1218160139)
What do you mean with "different chains"? `vSortedHeight` could contain blocks with the same height (forks), but I can't see how these could have made it into the block index without having a predecessor of height -1. Also, these shouldn't make the added check fail.
I do wonder though if it would be possible / more general to directly check that each block of height > 0 has a predecessor in the chain, instead of doing what the PR currently suggests. I'll look into that.
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1218160139)
What do you mean with "different chains"? `vSortedHeight` could contain blocks with the same height (forks), but I can't see how these could have made it into the block index without having a predecessor of height -1. Also, these shouldn't make the added check fail.
I do wonder though if it would be possible / more general to directly check that each block of height > 0 has a predecessor in the chain, instead of doing what the PR currently suggests. I'll look into that.
💬 mzumsande commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#issuecomment-1576922867)
> Now he won't have any info of what went wrong other than "Error loading block database", which can have multiple other reasons. Is there a way for us to communicate this better? Maybe a log? Or can we throw here?
Thanks! Yes, adding a log seems like a good idea, I'll do that with the next push!
(https://github.com/bitcoin/bitcoin/pull/27823#issuecomment-1576922867)
> Now he won't have any info of what went wrong other than "Error loading block database", which can have multiple other reasons. Is there a way for us to communicate this better? Maybe a log? Or can we throw here?
Thanks! Yes, adding a log seems like a good idea, I'll do that with the next push!
💬 fanquake commented on pull request "ci: enable AArch64 target in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27824#discussion_r1218175581)
> if that is possible?
Done with `Native`.
(https://github.com/bitcoin/bitcoin/pull/27824#discussion_r1218175581)
> if that is possible?
Done with `Native`.
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218179793)
Ok understood. In that case, would it make sense to be very explicit about what this connection is about? I mean user agent could essentially be "I am going to send you one TX and then disconnect". Additional behavior could even be encoded on the recipient side, maybe to ensure we don't disconnect prematurely ?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1218179793)
Ok understood. In that case, would it make sense to be very explicit about what this connection is about? I mean user agent could essentially be "I am going to send you one TX and then disconnect". Additional behavior could even be encoded on the recipient side, maybe to ensure we don't disconnect prematurely ?
💬 hebasto commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1576941978)
https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214342650:
> I think the approach in the branch is better for two reasons...
I agree with both of them.
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1576941978)
https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214342650:
> I think the approach in the branch is better for two reasons...
I agree with both of them.
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1576942166)
General question about this: what will be the impact on tor/i2p nodes? are there even enough hidden services nodes with inbound slots to support this? In a world where all TXs are broadcast to the network through "exit nodes" are there any other possible drawbacks ?
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1576942166)
General question about this: what will be the impact on tor/i2p nodes? are there even enough hidden services nodes with inbound slots to support this? In a world where all TXs are broadcast to the network through "exit nodes" are there any other possible drawbacks ?
👍 Sjors approved a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1462675329)
utACK c79539fc20d902b5f9e098cc996c4e27c8e5b8c5
Everything since my last review is a rebase, except 95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94, so I only re-reviewed that, plus 70a2bb75442daaef5685728b4b03c7b2fe49fd38.
I'd still like to see a test and/or release note to cover the case where a user upgrades, then downgrades and encrypts their wallet and then upgrades. See https://github.com/bitcoin/bitcoin/pull/26728/commits/95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94#r1081289252 But it can wait
...
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1462675329)
utACK c79539fc20d902b5f9e098cc996c4e27c8e5b8c5
Everything since my last review is a rebase, except 95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94, so I only re-reviewed that, plus 70a2bb75442daaef5685728b4b03c7b2fe49fd38.
I'd still like to see a test and/or release note to cover the case where a user upgrades, then downgrades and encrypts their wallet and then upgrades. See https://github.com/bitcoin/bitcoin/pull/26728/commits/95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94#r1081289252 But it can wait
...
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218151088)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94 `// Automatically`
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218151088)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94 `// Automatically`
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218166104)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: can we emit a warning the in the `else` case?
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218166104)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: can we emit a warning the in the `else` case?
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218161082)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: why not loop over `xpubs`?
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1218161082)
95d11f09c2c0d4ad66ba2e4ce2d15be4c002fb94: why not loop over `xpubs`?