💬 maflcko commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1591973597)
Any reason to drop the ParseHex, which allows to write this as a string literal?
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1591973597)
Any reason to drop the ParseHex, which allows to write this as a string literal?
🤔 ismaelsadeeq reviewed a pull request: "doc: fix broken relative md links"
(https://github.com/bitcoin/bitcoin/pull/30025#pullrequestreview-2042354354)
Re ACK 4b9f49da2b120e81516ddc3dc577d7a2e58e02d3
(https://github.com/bitcoin/bitcoin/pull/30025#pullrequestreview-2042354354)
Re ACK 4b9f49da2b120e81516ddc3dc577d7a2e58e02d3
💬 ajtowns commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592014682)
Oh, secp256k1-zkp has a straight forward description: https://github.com/BlockstreamResearch/secp256k1-zkp/blob/11af7015de624b010424273be3d91f117f172c82/src/modules/rangeproof/main_impl.h#L16
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592014682)
Oh, secp256k1-zkp has a straight forward description: https://github.com/BlockstreamResearch/secp256k1-zkp/blob/11af7015de624b010424273be3d91f117f172c82/src/modules/rangeproof/main_impl.h#L16
💬 vasild commented on issue "Could the wallet count unconfirmed non-mempool change?":
(https://github.com/bitcoin/bitcoin/issues/11887#issuecomment-2097746943)
> Related to private broadcast @vasild ?
Right. Mentioned at the bottom of OP in https://github.com/bitcoin/bitcoin/pull/29415:
> Next step after this PR is done will be to have the wallet do the private broadcast as well, https://github.com/bitcoin/bitcoin/issues/11887 would have to be resolved.
(https://github.com/bitcoin/bitcoin/issues/11887#issuecomment-2097746943)
> Related to private broadcast @vasild ?
Right. Mentioned at the bottom of OP in https://github.com/bitcoin/bitcoin/pull/29415:
> Next step after this PR is done will be to have the wallet do the private broadcast as well, https://github.com/bitcoin/bitcoin/issues/11887 would have to be resolved.
💬 maflcko commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#issuecomment-2097780769)
Not sure if there is much value in being overly accurate here (`clang-15 and libc++-16` or `clang-16 and libstdc++`). If someone can install clang-16 or libc++-16, they can probably also install the other as well?
So I'd say to close this and bump to 16 wholesale?
(https://github.com/bitcoin/bitcoin/pull/29077#issuecomment-2097780769)
Not sure if there is much value in being overly accurate here (`clang-15 and libc++-16` or `clang-16 and libstdc++`). If someone can install clang-16 or libc++-16, they can probably also install the other as well?
So I'd say to close this and bump to 16 wholesale?
💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1592074394)
:doh: yes! fixed and added unit tests for the `NO_LIMIT` case.
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1592074394)
:doh: yes! fixed and added unit tests for the `NO_LIMIT` case.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2097797401)
Rebased for #29984
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2097797401)
Rebased for #29984
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1592075348)
Yes, good point.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1592075348)
Yes, good point.
✅ vasild closed a pull request: "addrman: treat Tor/I2P/CJDNS as a single group"
(https://github.com/bitcoin/bitcoin/pull/22563)
(https://github.com/bitcoin/bitcoin/pull/22563)
💬 vasild commented on pull request "addrman: treat Tor/I2P/CJDNS as a single group":
(https://github.com/bitcoin/bitcoin/pull/22563#issuecomment-2097799927)
Closing this because I think the proper solution is a bigger change as mentioned above in https://github.com/bitcoin/bitcoin/pull/22563#issuecomment-1717157204:
_If the address is IPv4 or IPv6 then use `GetGroup()`
Else do something else (for Tor, I2P and CJDNS addresses)._
Currently `GetGroup()` is called for all types of addresses from:
`AddrInfo::GetTriedBucket()`
`AddrInfo::GetNewBucket()`
`CConnman::CalculateKeyedNetGroup() // saved in CNode::nKeyedNetGroup and later used during e
...
(https://github.com/bitcoin/bitcoin/pull/22563#issuecomment-2097799927)
Closing this because I think the proper solution is a bigger change as mentioned above in https://github.com/bitcoin/bitcoin/pull/22563#issuecomment-1717157204:
_If the address is IPv4 or IPv6 then use `GetGroup()`
Else do something else (for Tor, I2P and CJDNS addresses)._
Currently `GetGroup()` is called for all types of addresses from:
`AddrInfo::GetTriedBucket()`
`AddrInfo::GetNewBucket()`
`CConnman::CalculateKeyedNetGroup() // saved in CNode::nKeyedNetGroup and later used during e
...
💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592086885)
Perfect, will use this!
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592086885)
Perfect, will use this!
💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592095735)
Seemed unnecessary? Representing this as a byte vector allows us to create an `XOnlyPublicKey` directly without an extra function call. Is there a reason to prefer having this as a string literal?
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592095735)
Seemed unnecessary? Representing this as a byte vector allows us to create an `XOnlyPublicKey` directly without an extra function call. Is there a reason to prefer having this as a string literal?
💬 hebasto commented on pull request "build, test, doc: Temporarily remove Android-related stuff":
(https://github.com/bitcoin/bitcoin/pull/30049#issuecomment-2097841736)
> I guess the idea is that there's basically nothing worth salvaging from our current Android build procedure?
The commit history keeps everything we might need when restoring Android builds.
(https://github.com/bitcoin/bitcoin/pull/30049#issuecomment-2097841736)
> I guess the idea is that there's basically nothing worth salvaging from our current Android build procedure?
The commit history keeps everything we might need when restoring Android builds.
💬 maflcko commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592108480)
Just a style-nit, as it makes it easier to grep and compare the string literal. The function call could be moved to compile time, but I haven't implemented it yet. Calling it once at runtime should be fine also, for now?
Just a nit in any case.
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592108480)
Just a style-nit, as it makes it easier to grep and compare the string literal. The function call could be moved to compile time, but I haven't implemented it yet. Calling it once at runtime should be fine also, for now?
Just a nit in any case.
💬 josibake commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2097902146)
> This function feels kinda weird. As it's named `ApplyTapTweak`, I would expect it to modify `*this`. It also takes in a `uint256* merkle_root` but doesn't check for null, a reference would make more sense.
>
> Perhaps this would make more sense as a static utility function that takes input/output keys?
Utility function might make more sense here: could do a utility function with `secp256k1_keypairs` as in/out args:
```
int compute_taptweak(secp256k1_keypair in, unsigned char merkle_r
...
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2097902146)
> This function feels kinda weird. As it's named `ApplyTapTweak`, I would expect it to modify `*this`. It also takes in a `uint256* merkle_root` but doesn't check for null, a reference would make more sense.
>
> Perhaps this would make more sense as a static utility function that takes input/output keys?
Utility function might make more sense here: could do a utility function with `secp256k1_keypairs` as in/out args:
```
int compute_taptweak(secp256k1_keypair in, unsigned char merkle_r
...
💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592155826)
Ah cool, my slight aversion to `ParseHex` was the call at runtime for a static const, but if we are going to move that to compile time and the string literal makes it easier to compare (e.g. with the value in BIP341, which is also a string literal), then happy to change it.
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592155826)
Ah cool, my slight aversion to `ParseHex` was the call at runtime for a static const, but if we are going to move that to compile time and the string literal makes it easier to compare (e.g. with the value in BIP341, which is also a string literal), then happy to change it.
💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2097928134)
Added a comment per @ajtowns 's feedback and reverted to using `ParseHex` on a string literal per @maflcko 's feedback.
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2097928134)
Added a comment per @ajtowns 's feedback and reverted to using `ParseHex` on a string literal per @maflcko 's feedback.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2097968387)
> That said, it is a little rough to review as
> It needs to be compared to the letter of the spec, with the assumption that @laanwj is evil or has mis-implemented (I doubt that :)
If you're more comfortable comparing it against another implementation there's:
- A kinda haphazard windows implementation here: https://github.com/moonlight-stream/GS-IPv6-Forwarder/blob/master/GSv6Fwd/pcp.cpp
- Miniupnpd's pcp server implementation https://github.com/miniupnp/miniupnp/blob/master/miniupnpd/pc
...
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2097968387)
> That said, it is a little rough to review as
> It needs to be compared to the letter of the spec, with the assumption that @laanwj is evil or has mis-implemented (I doubt that :)
If you're more comfortable comparing it against another implementation there's:
- A kinda haphazard windows implementation here: https://github.com/moonlight-stream/GS-IPv6-Forwarder/blob/master/GSv6Fwd/pcp.cpp
- Miniupnpd's pcp server implementation https://github.com/miniupnp/miniupnp/blob/master/miniupnpd/pc
...
💬 craigraw commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1592183994)
Typo here - should be `Use the testnet4 chain.`
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1592183994)
Typo here - should be `Use the testnet4 chain.`
👍 rkrux approved a pull request: "test: added test coverage to loadtxoutset could not open file"
(https://github.com/bitcoin/bitcoin/pull/30053#pullrequestreview-2042752271)
ACK [ee67bba](https://github.com/bitcoin/bitcoin/pull/30053/commits/ee67bba76cca2355541f99bb731f58479981b29e)
Make successful, unit and functional tests successful.
Short and sweet!
(https://github.com/bitcoin/bitcoin/pull/30053#pullrequestreview-2042752271)
ACK [ee67bba](https://github.com/bitcoin/bitcoin/pull/30053/commits/ee67bba76cca2355541f99bb731f58479981b29e)
Make successful, unit and functional tests successful.
Short and sweet!