💬 fanquake commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090522477)
> Mind that this adds an utility and is orthogonal to UPnP.
~0. Right, I have misunderstood what this is/does. This could be a useful tool (for some % of users of this software), but I don't think this repository is the right place for it to live, or that it fits into the set of things we should be maintaining here.
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090522477)
> Mind that this adds an utility and is orthogonal to UPnP.
~0. Right, I have misunderstood what this is/does. This could be a useful tool (for some % of users of this software), but I don't think this repository is the right place for it to live, or that it fits into the set of things we should be maintaining here.
💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090546884)
This is a temporary PR for people to test this code, i intend to integrate it in bitcoind just like the current IPv4 port mapping stuff. I did not intend it to be a useful tool to keep around. This is why the title says "nomerge".
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090546884)
This is a temporary PR for people to test this code, i intend to integrate it in bitcoind just like the current IPv4 port mapping stuff. I did not intend it to be a useful tool to keep around. This is why the title says "nomerge".
💬 fanquake commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090550845)
> I did not intend it to be a useful tool to keep around.
Fair enough, I'll -redirect my NACK to this thread: https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587310888.
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090550845)
> I did not intend it to be a useful tool to keep around.
Fair enough, I'll -redirect my NACK to this thread: https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587310888.
💬 cygnet3 commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2090551114)
Quickly made a tool to get the tweaks for a block using block height or block hash [here](https://github.com/cygnet3/sp-scan-block)
`$ cargo run --release -- --blkheight 800000`
```json
[
"02740690cbe477ffcd2cf84a7bcb65a41adce26436758b00a58f3901605c3376eb",
"03f8470afaceb6946f3a01a37ecade692e8370f5cc9b2d1ff2edb7a3b01e857a3e",
"..."
]
```
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2090551114)
Quickly made a tool to get the tweaks for a block using block height or block hash [here](https://github.com/cygnet3/sp-scan-block)
`$ cargo run --release -- --blkheight 800000`
```json
[
"02740690cbe477ffcd2cf84a7bcb65a41adce26436758b00a58f3901605c3376eb",
"03f8470afaceb6946f3a01a37ecade692e8370f5cc9b2d1ff2edb7a3b01e857a3e",
"..."
]
```
💬 fanquake commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587671315)
> You could make this a permanent debugging utility by adding it to bitcoin-util.
Concept NACK.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587671315)
> You could make this a permanent debugging utility by adding it to bitcoin-util.
Concept NACK.
💬 willcl-ark commented on pull request "Fix misleading signmessage error with segwit":
(https://github.com/bitcoin-core/gui/pull/819#issuecomment-2090565799)
Hi @hebasto I took your suggestions save for the extra context comments -- I think the text as one blob like this is pretty self-explanatory?
(https://github.com/bitcoin-core/gui/pull/819#issuecomment-2090565799)
Hi @hebasto I took your suggestions save for the extra context comments -- I think the text as one blob like this is pretty self-explanatory?
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090577873)
> Sure, could. But mind that IPv4 will be mostly a different path.
I see. I guess future UI confusion can ben prevented by calling this "PCP" with a not saying that it works with IPv6 only for now.
The GUI could also hide the kitchen sink of methods under some advanced toggle. In any case that doesn't affect testing.
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090577873)
> Sure, could. But mind that IPv4 will be mostly a different path.
I see. I guess future UI confusion can ben prevented by calling this "PCP" with a not saying that it works with IPv6 only for now.
The GUI could also hide the kitchen sink of methods under some advanced toggle. In any case that doesn't affect testing.
💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587694422)
> The argument I'm making in this PR is that in all the places we are checking by txid, we shouldn't be doing that, because we're missing same-txid-different-witness cases.
I agree, the rationale makes sense. My concern is about segwit txs (`txid != wtxid`) where we check by txid (for whatever reason: intentional because we don't have witness data, legacy and we never updated it, buggy, ...): just creating a wtxid from the txid hash as is done here seems like it has potential to start missing
...
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587694422)
> The argument I'm making in this PR is that in all the places we are checking by txid, we shouldn't be doing that, because we're missing same-txid-different-witness cases.
I agree, the rationale makes sense. My concern is about segwit txs (`txid != wtxid`) where we check by txid (for whatever reason: intentional because we don't have witness data, legacy and we never updated it, buggy, ...): just creating a wtxid from the txid hash as is done here seems like it has potential to start missing
...
💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587705035)
To be clear: I'm not saying this PR needs to overhaul `AlreadyHaveTx` like I did, and I think having `TxOrphanage` index on `Wtxid` instead of `Txid` is better. I just think that if we're going to be calling `TxOrphanage` methods with (what are in fact) txids, we should probably have `TxOrphanage` _also_ keep a `Txid` index and not to blind conversion such as is happening here? I think that would address my concerns and make things easier to review?
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587705035)
To be clear: I'm not saying this PR needs to overhaul `AlreadyHaveTx` like I did, and I think having `TxOrphanage` index on `Wtxid` instead of `Txid` is better. I just think that if we're going to be calling `TxOrphanage` methods with (what are in fact) txids, we should probably have `TxOrphanage` _also_ keep a `Txid` index and not to blind conversion such as is happening here? I think that would address my concerns and make things easier to review?
💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587705703)
fwiw, I think the in-orphanage txid==wtixd case seems pretty rare all things considered. I had 4 over a 24 hour period, vs about 400 times where we fetched a segwit parent via txid though that particular txid was in the orphanage already.
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587705703)
fwiw, I think the in-orphanage txid==wtixd case seems pretty rare all things considered. I had 4 over a 24 hour period, vs about 400 times where we fetched a segwit parent via txid though that particular txid was in the orphanage already.
📝 fanquake locked a pull request: "Implement BIP 118 validation (SIGHASH_ANYPREVOUT)"
(https://github.com/bitcoin/bitcoin/pull/30018)
Forward port of #34
(https://github.com/bitcoin/bitcoin/pull/30018)
Forward port of #34
💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090624771)
> I see. I guess future UI confusion can ben prevented by calling this "PCP" with a note saying that it works with IPv6 only for now.
OK, i see your point now. Let's replace NAT-PMP for IPv4 at the same time, so we keep the same number of options, and lose one dependency.
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090624771)
> I see. I guess future UI confusion can ben prevented by calling this "PCP" with a note saying that it works with IPv6 only for now.
OK, i see your point now. Let's replace NAT-PMP for IPv4 at the same time, so we keep the same number of options, and lose one dependency.
💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587730485)
> iow I'm not sure extra complexity is worth it,
Oh, fair enough. I don't have a good enough view of the attack vectors here. In my review, I was operating on the assumption that we wouldn't want to miss anything we're currently doing. If it's okay to relax that, that could make sense - but it should probably be made explicit then?
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587730485)
> iow I'm not sure extra complexity is worth it,
Oh, fair enough. I don't have a good enough view of the attack vectors here. In my review, I was operating on the assumption that we wouldn't want to miss anything we're currently doing. If it's okay to relax that, that could make sense - but it should probably be made explicit then?
👋 fanquake's pull request is ready for review: "build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set"
(https://github.com/bitcoin/bitcoin/pull/25972)
(https://github.com/bitcoin/bitcoin/pull/25972)
💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587742487)
IIUC If the check is removed, you'll waste bandwidth when you get orphan chains that are non-segwit, but you won't keep or relay them, so it shouldn't churn the orphanage.
Would it help if the comment was touched up to be explicit about what it's saving us from doing?
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587742487)
IIUC If the check is removed, you'll waste bandwidth when you get orphan chains that are non-segwit, but you won't keep or relay them, so it shouldn't churn the orphanage.
Would it help if the comment was touched up to be explicit about what it's saving us from doing?
📝 jasonandjay opened a pull request: "doc: add dustThreshold explain of P2SH"
(https://github.com/bitcoin/bitcoin/pull/30023)
The calculation of dust in Bitcoin has different values for different payment types.
For non-Witness: (148 + vout size)*rate
For Witness: (67+vout size)*rate
According to our calculation logic, our common P2PKH is 546, P2SH is 540, the minimum value of Witness is P2WPKH is 294, and P2TR is 330
However, the comments only mentioned P2PKH’s 546 and P2WPKH’s 294, which is inconsistent with the code logic.
These four payment types are very common in BTC wallets, so I think it is necessary
...
(https://github.com/bitcoin/bitcoin/pull/30023)
The calculation of dust in Bitcoin has different values for different payment types.
For non-Witness: (148 + vout size)*rate
For Witness: (67+vout size)*rate
According to our calculation logic, our common P2PKH is 546, P2SH is 540, the minimum value of Witness is P2WPKH is 294, and P2TR is 330
However, the comments only mentioned P2PKH’s 546 and P2WPKH’s 294, which is inconsistent with the code logic.
These four payment types are very common in BTC wallets, so I think it is necessary
...
💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1587753988)
What's the reasoning for raising the minimum difficulty to 1 million? This will effectively prevent a developer from being able to move the chain forward without an ASIC.
I'd think that closing the timewarp loophole + the difficulty adjustment loophole should be sufficient to prevent block storms without actually making it more difficult for a dev to mint a low cost block if the chain has stalled.
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1587753988)
What's the reasoning for raising the minimum difficulty to 1 million? This will effectively prevent a developer from being able to move the chain forward without an ASIC.
I'd think that closing the timewarp loophole + the difficulty adjustment loophole should be sufficient to prevent block storms without actually making it more difficult for a dev to mint a low cost block if the chain has stalled.
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090792385)
> I'm really suprised there isn't software that already exists, for doing the same thing, that we could just point users to?
If there's a library that does the same thing, and doesn't have 1 million lines of code, then it seems fine to include it. We can also make it a library under the core repo, which perhaps other projects will find useful for its simplicity. At least now I have a sense of what that library should be doing.
I don't think we should ask users to install a separate piece o
...
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090792385)
> I'm really suprised there isn't software that already exists, for doing the same thing, that we could just point users to?
If there's a library that does the same thing, and doesn't have 1 million lines of code, then it seems fine to include it. We can also make it a library under the core repo, which perhaps other projects will find useful for its simplicity. At least now I have a sense of what that library should be doing.
I don't think we should ask users to install a separate piece o
...
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587827580)
No need to add networking indeed. As long as the eventual implementation (library or otherwise) has proper debug logging, we probably won't need a standalone tool for that.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587827580)
No need to add networking indeed. As long as the eventual implementation (library or otherwise) has proper debug logging, we probably won't need a standalone tool for that.
🤔 glozow reviewed a pull request: "doc: add dustThreshold explain of P2SH & P2TR"
(https://github.com/bitcoin/bitcoin/pull/30023#pullrequestreview-2036066205)
Thanks for your interest in contributing, but this comment doesn't reflect what the code does (also see #22779 which is linked in a comment in the code). I think this could just be confusing?
(https://github.com/bitcoin/bitcoin/pull/30023#pullrequestreview-2036066205)
Thanks for your interest in contributing, but this comment doesn't reflect what the code does (also see #22779 which is linked in a comment in the code). I think this could just be confusing?