💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587621612)
`Sock` is used below, which is the mockable socket. The only exception is construction, so yea for testing we'd want a function where the Sock is passed in.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587621612)
`Sock` is used below, which is the mockable socket. The only exception is construction, so yea for testing we'd want a function where the Sock is passed in.
💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587626026)
It might be useful for troubleshooting, though i'm not sure how happy people will be to add networking stuff to bitcoin-util. Currently it's pure-function stuff only 😄
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587626026)
It might be useful for troubleshooting, though i'm not sure how happy people will be to add networking stuff to bitcoin-util. Currently it's pure-function stuff only 😄
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090487679)
> i'm kind of hestitant to touch that, given how hard it already is to find people to test these kind of things
For now we could add support in the test, but stick to IPv6 in the implementation. OTOH both UPnP and NAP-PMP are off by default. Adding another off-by-default checkbox "IPv6 pinhole" to the GUI seems pretty confusing, and probably doesn't increase the chance of users trying this.
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090487679)
> i'm kind of hestitant to touch that, given how hard it already is to find people to test these kind of things
For now we could add support in the test, but stick to IPv6 in the implementation. OTOH both UPnP and NAP-PMP are off by default. Adding another off-by-default checkbox "IPv6 pinhole" to the GUI seems pretty confusing, and probably doesn't increase the chance of users trying this.
💬 fanquake commented on pull request "build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set":
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-2090498778)
This will likely fail one last time, but should be good after #30012.
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-2090498778)
This will likely fail one last time, but should be good after #30012.
💬 instagibbs commented on pull request "opportunistic 1p1c followups":
(https://github.com/bitcoin/bitcoin/pull/30012#issuecomment-2090502979)
reACK https://github.com/bitcoin/bitcoin/pull/30012/commits/7f6fb73c82fdff2afe5edefcc335ba6707d5627d
just the non-pointer copying added
(https://github.com/bitcoin/bitcoin/pull/30012#issuecomment-2090502979)
reACK https://github.com/bitcoin/bitcoin/pull/30012/commits/7f6fb73c82fdff2afe5edefcc335ba6707d5627d
just the non-pointer copying added
💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090509155)
> Could you rebase this on master, now that we are using the latest version of miniupnpc (https://github.com/bitcoin/bitcoin/pull/29707).
Yes, but first want to address @Sjors's comments. Mind that this adds an utility and is orthogonal to UPnP. Adding IPv6 pinholing through UPnP would be useful too (for routers that support that, and not PCP), so we'll likely want to be able to do both in the eventual design like we do for natpmp/UPnP for IPv4.
> For now we could add support in the test,
...
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090509155)
> Could you rebase this on master, now that we are using the latest version of miniupnpc (https://github.com/bitcoin/bitcoin/pull/29707).
Yes, but first want to address @Sjors's comments. Mind that this adds an utility and is orthogonal to UPnP. Adding IPv6 pinholing through UPnP would be useful too (for routers that support that, and not PCP), so we'll likely want to be able to do both in the eventual design like we do for natpmp/UPnP for IPv4.
> For now we could add support in the test,
...
👍 brunoerg approved a pull request: "contrib: Add asmap-tool"
(https://github.com/bitcoin/bitcoin/pull/28793#pullrequestreview-2035758814)
ACK 6abe772a17e09fe96e68cd3311280d5a30f6378b
I've been using this for a while but did some new tests to ensure.
I've tested encoding/decoding as well as the diff command with a very old file (1 year ago) and a newest one to test big changes - e.g.: `# 1296418840 (2^30.27) IPv4 addresses changed; 8741211000446919691919144517632603 (2^112.75) IPv6 addresses changed).`. I think code could be clearer but lgtm to merge as is.
(https://github.com/bitcoin/bitcoin/pull/28793#pullrequestreview-2035758814)
ACK 6abe772a17e09fe96e68cd3311280d5a30f6378b
I've been using this for a while but did some new tests to ensure.
I've tested encoding/decoding as well as the diff command with a very old file (1 year ago) and a newest one to test big changes - e.g.: `# 1296418840 (2^30.27) IPv4 addresses changed; 8741211000446919691919144517632603 (2^112.75) IPv6 addresses changed).`. I think code could be clearer but lgtm to merge as is.
💬 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?