💬 maflcko commented on issue "test: Intermittent issue in feature_init.py", line 88, in run_test with node.wait_for_debug_log([terminate_line]): AssertionError: [node 0] Expected messages "[b'scheduler thread start']" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/30011#issuecomment-2097572483)
Possibly it started to happen after https://github.com/bitcoin/bitcoin/pull/29848 ?
(https://github.com/bitcoin/bitcoin/issues/30011#issuecomment-2097572483)
Possibly it started to happen after https://github.com/bitcoin/bitcoin/pull/29848 ?
💬 maflcko commented on issue "test: Intermittent issue in feature_init.py", line 88, in run_test with node.wait_for_debug_log([terminate_line]): AssertionError: [node 0] Expected messages "[b'scheduler thread start']" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/30011#issuecomment-2097573565)
ugh https://github.com/bitcoin/bitcoin/pull/29848#discussion_r1563849606
(https://github.com/bitcoin/bitcoin/issues/30011#issuecomment-2097573565)
ugh https://github.com/bitcoin/bitcoin/pull/29848#discussion_r1563849606
📝 maflcko opened a pull request: "ci: Exclude feature_init for now in valgrind task"
(https://github.com/bitcoin/bitcoin/pull/30054)
Fixes https://github.com/bitcoin/bitcoin/issues/30011
(https://github.com/bitcoin/bitcoin/pull/30054)
Fixes https://github.com/bitcoin/bitcoin/issues/30011
💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1591963853)
Added a comment for where the point comes from. Tried to write a sentence for the "why" but don't really understand it well enough to articulate succinctly :sweat_smile:.
From what I understand, because `H` is derived from a known public value `G` via a hash, we learn nothing about the discrete log of `H` due to the hash function providing us a uniformly random value. But not sure if that's entirely correct; if you have a suggestion on how to better (or more correctly!) phrase this, happy to
...
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1591963853)
Added a comment for where the point comes from. Tried to write a sentence for the "why" but don't really understand it well enough to articulate succinctly :sweat_smile:.
From what I understand, because `H` is derived from a known public value `G` via a hash, we learn nothing about the discrete log of `H` due to the hash function providing us a uniformly random value. But not sure if that's entirely correct; if you have a suggestion on how to better (or more correctly!) phrase this, happy to
...
💬 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.