💬 rkrux commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079666382)
I can see the earlier implementation also ended the iteration at second index (1), which is ok.
However, the `+ 1` is quite apparent now & I'm not entirely sure why this is the case.
It would be helpful to know the reason and document it in code if possible.
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079666382)
I can see the earlier implementation also ended the iteration at second index (1), which is ok.
However, the `+ 1` is quite apparent now & I'm not entirely sure why this is the case.
It would be helpful to know the reason and document it in code if possible.
💬 fanquake commented on pull request "depends: Avoid using helper variables in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2863061380)
Guix Build:
```bash
cab0e4d3d2602d2eaa8893ae5294bd736b514fea6deb72df03c4d693971282dd guix-build-7343a1846ceb/output/aarch64-linux-gnu/SHA256SUMS.part
21163373fc74e834f3d81309888fc4cd99719462be8fef447e535fb56aa5ce1e guix-build-7343a1846ceb/output/aarch64-linux-gnu/bitcoin-7343a1846ceb-aarch64-linux-gnu-debug.tar.gz
b09ca7a33bf16d3e97bce8800bf57a06402a9cf4860d327be3d6571a56be598c guix-build-7343a1846ceb/output/aarch64-linux-gnu/bitcoin-7343a1846ceb-aarch64-linux-gnu.tar.gz
f1b2aa3edd0ce6b4
...
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2863061380)
Guix Build:
```bash
cab0e4d3d2602d2eaa8893ae5294bd736b514fea6deb72df03c4d693971282dd guix-build-7343a1846ceb/output/aarch64-linux-gnu/SHA256SUMS.part
21163373fc74e834f3d81309888fc4cd99719462be8fef447e535fb56aa5ce1e guix-build-7343a1846ceb/output/aarch64-linux-gnu/bitcoin-7343a1846ceb-aarch64-linux-gnu-debug.tar.gz
b09ca7a33bf16d3e97bce8800bf57a06402a9cf4860d327be3d6571a56be598c guix-build-7343a1846ceb/output/aarch64-linux-gnu/bitcoin-7343a1846ceb-aarch64-linux-gnu.tar.gz
f1b2aa3edd0ce6b4
...
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2079717534)
I feel it's fine to keep them - imho the ultimate test is whether the transaction can be mined, gives me more confidence.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2079717534)
I feel it's fine to keep them - imho the ultimate test is whether the transaction can be mined, gives me more confidence.
💬 laanwj commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2079719628)
Would it be possible to work around this in the build script somehow?
i guess not, i mean except by changing a global git setting there, which would be awful behavior...
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2079719628)
Would it be possible to work around this in the build script somehow?
i guess not, i mean except by changing a global git setting there, which would be awful behavior...
💬 laanwj commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079728574)
The reason is that the first byte cannot have the upper bit set, as this would mean a negative number, which would be illegal here. So the number can be 33 bytes long (with a 0x00 prefixed) even though it fits in 32 bytes.
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079728574)
The reason is that the first byte cannot have the upper bit set, as this would mean a negative number, which would be illegal here. So the number can be 33 bytes long (with a 0x00 prefixed) even though it fits in 32 bytes.
💬 vasild commented on issue "rfc: only relay transactions to v2 encrypted peers":
(https://github.com/bitcoin/bitcoin/issues/32373#issuecomment-2863104253)
I agree with @mzumsande above, also:
If a node has even a single v2 connection that already makes it nearly impossible for a passive spying ISP to conclude it is the origin of a transaction, no?
Lets assume a node has only v1 connections, then the ISP can observe all transactions going to it and all transactions going out of it. As soon as a transaction is seen going out that did not come in, a conclusion can be made that it originated from the node.
However, if there are v2 connections, even
...
(https://github.com/bitcoin/bitcoin/issues/32373#issuecomment-2863104253)
I agree with @mzumsande above, also:
If a node has even a single v2 connection that already makes it nearly impossible for a passive spying ISP to conclude it is the origin of a transaction, no?
Lets assume a node has only v1 connections, then the ISP can observe all transactions going to it and all transactions going out of it. As soon as a transaction is seen going out that did not come in, a conclusion can be made that it originated from the node.
However, if there are v2 connections, even
...
💬 theuni commented on pull request "crypto: disable ASan for sha256_sse4 with Clang":
(https://github.com/bitcoin/bitcoin/pull/32437#issuecomment-2863107504)
Post-merge ACK 4e8ab5e00fa72016a7ec0e0505ca025d4e59e4d8. Looks like the right fix to me.
(https://github.com/bitcoin/bitcoin/pull/32437#issuecomment-2863107504)
Post-merge ACK 4e8ab5e00fa72016a7ec0e0505ca025d4e59e4d8. Looks like the right fix to me.
📝 fanquake opened a pull request: "contrib: remove bdb exception from FORTIFY check"
(https://github.com/bitcoin/bitcoin/pull/32448)
BDB has been removed (#28710), so we no-longer need to ignore functions from BDB in this check.
Guix building this branch, and looking for `*_chk` functions across all binaries produces:
```
# nm -C * | grep -i _chk | sort | uniq
U __fdelt_chk@GLIBC_2.15
U __fprintf_chk@GLIBC_2.3.4
U __fread_chk@GLIBC_2.7
U __longjmp_chk@GLIBC_2.11
U __memcpy_chk@GLIBC_2.3.4
U __printf_chk@GLIBC_2.3.4
...
(https://github.com/bitcoin/bitcoin/pull/32448)
BDB has been removed (#28710), so we no-longer need to ignore functions from BDB in this check.
Guix building this branch, and looking for `*_chk` functions across all binaries produces:
```
# nm -C * | grep -i _chk | sort | uniq
U __fdelt_chk@GLIBC_2.15
U __fprintf_chk@GLIBC_2.3.4
U __fread_chk@GLIBC_2.7
U __longjmp_chk@GLIBC_2.11
U __memcpy_chk@GLIBC_2.3.4
U __printf_chk@GLIBC_2.3.4
...
💬 purpleKarrot commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2863122404)
When someone builds from a tarball that is more than a year old, this will use the current year instead of the year of the release. Is that a valid use case? Otherwise, ACK dfeac76b951fc2d28e0832b176f3c8ad595697f1.
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2863122404)
When someone builds from a tarball that is more than a year old, this will use the current year instead of the year of the release. Is that a valid use case? Otherwise, ACK dfeac76b951fc2d28e0832b176f3c8ad595697f1.
💬 hebasto commented on issue "32-bit Linux: build flags lost with depends & overriden CC(X)":
(https://github.com/bitcoin/bitcoin/issues/28096#issuecomment-2863226724)
I've just re-checked on Ubuntu 25.04 for the master branch @ 66c968b4b4a98b1d476cb79b0dd2f6b09420c945:
```
$ sudo apt install g++-multilib
$ gmake -C depends -j $(nproc) HOST=i686-pc-linux-gnu NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_USDT=1 CC=clang CXX=clang++
$ cmake -B build --toolchain depends/i686-pc-linux-gnu/toolchain.cmake
$ cmake --build build -t bitcoind
$ file build/bin/bitcoind
build/bin/bitcoind: ELF 32-bit LSB pie executable, Intel 80386, version 1 (SYSV), dynamically linked, interpreter /
...
(https://github.com/bitcoin/bitcoin/issues/28096#issuecomment-2863226724)
I've just re-checked on Ubuntu 25.04 for the master branch @ 66c968b4b4a98b1d476cb79b0dd2f6b09420c945:
```
$ sudo apt install g++-multilib
$ gmake -C depends -j $(nproc) HOST=i686-pc-linux-gnu NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_USDT=1 CC=clang CXX=clang++
$ cmake -B build --toolchain depends/i686-pc-linux-gnu/toolchain.cmake
$ cmake --build build -t bitcoind
$ file build/bin/bitcoind
build/bin/bitcoind: ELF 32-bit LSB pie executable, Intel 80386, version 1 (SYSV), dynamically linked, interpreter /
...
💬 rkrux commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079813981)
Interesting, thanks. I did read this code comment earlier but the above comment is helpful in connecting the dots.
https://github.com/bitcoin/bitcoin/blob/66c968b4b4a98b1d476cb79b0dd2f6b09420c945/src/key.cpp#L201-L204
> Always padding to 33 bytes then applying it to data()+1 is just a convenience
Re: `Always padding` - In the case the first bit is not set already, even then prepending with 00 makes it easier to grasp.
Otherwise, `s[0] == 0` in some cases, and `< 0x80` in the other cases
...
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079813981)
Interesting, thanks. I did read this code comment earlier but the above comment is helpful in connecting the dots.
https://github.com/bitcoin/bitcoin/blob/66c968b4b4a98b1d476cb79b0dd2f6b09420c945/src/key.cpp#L201-L204
> Always padding to 33 bytes then applying it to data()+1 is just a convenience
Re: `Always padding` - In the case the first bit is not set already, even then prepending with 00 makes it easier to grasp.
Otherwise, `s[0] == 0` in some cases, and `< 0x80` in the other cases
...
💬 theStack commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079824869)
Added a comment.
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079824869)
Added a comment.
📝 portlandhodl converted_to_draft a pull request: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260)
### **Issue:**
Simply `DecodeDestination` does not handle errors where the user inputs a valid address for the wrong network. _e.x. testnet while running client on mainnet_
The current error `not a valid Bech32 or Base58 encoding` for a valid address on a different network is entirely incorrect. This is because of the is_bech32 variable used at the core of `DecodeDestination` logic only checks that the prefix matches. If it doesn't it just starts running everything as `DecodeBase58Check
...
(https://github.com/bitcoin/bitcoin/pull/27260)
### **Issue:**
Simply `DecodeDestination` does not handle errors where the user inputs a valid address for the wrong network. _e.x. testnet while running client on mainnet_
The current error `not a valid Bech32 or Base58 encoding` for a valid address on a different network is entirely incorrect. This is because of the is_bech32 variable used at the core of `DecodeDestination` logic only checks that the prefix matches. If it doesn't it just starts running everything as `DecodeBase58Check
...
👍 hodlinator approved a pull request: "doc: Improve `dependencies.md`"
(https://github.com/bitcoin/bitcoin/pull/31895#pullrequestreview-2825304323)
re-ACK cb623692a6089c39560a637a2bf064d54aebb4d4
Changes since [previous ACK](https://github.com/bitcoin/bitcoin/pull/31895#pullrequestreview-2796426546):
* Rebased after BDB dependency removal.
* Removed remaining "Version used" columns based off https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063356972.
(https://github.com/bitcoin/bitcoin/pull/31895#pullrequestreview-2825304323)
re-ACK cb623692a6089c39560a637a2bf064d54aebb4d4
Changes since [previous ACK](https://github.com/bitcoin/bitcoin/pull/31895#pullrequestreview-2796426546):
* Rebased after BDB dependency removal.
* Removed remaining "Version used" columns based off https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063356972.
💬 hodlinator commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2079815634)
nit: Feels weird to remove "Version used" column for runtime dependencies as part of the second commit (splitting build/runtime dependencies), but keeping them for buildtime dependencies until the end.
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2079815634)
nit: Feels weird to remove "Version used" column for runtime dependencies as part of the second commit (splitting build/runtime dependencies), but keeping them for buildtime dependencies until the end.
👍 theuni approved a pull request: "contrib: remove bdb exception from FORTIFY check"
(https://github.com/bitcoin/bitcoin/pull/32448#pullrequestreview-2825363625)
utACK f9dfe8d5e0d3f628659702ab781b7919505c829f
(https://github.com/bitcoin/bitcoin/pull/32448#pullrequestreview-2825363625)
utACK f9dfe8d5e0d3f628659702ab781b7919505c829f
💬 theStack commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079853332)
As another reference and explanation, see also the corresponding serialization implementation in the functional test framework: https://github.com/bitcoin/bitcoin/blob/66c968b4b4a98b1d476cb79b0dd2f6b09420c945/test/functional/test_framework/key.py#L184-L188
(https://github.com/bitcoin/bitcoin/pull/32436#discussion_r2079853332)
As another reference and explanation, see also the corresponding serialization implementation in the functional test framework: https://github.com/bitcoin/bitcoin/blob/66c968b4b4a98b1d476cb79b0dd2f6b09420c945/test/functional/test_framework/key.py#L184-L188
💬 AlexSQY commented on pull request "CAT in Tapscript (BIP-347)":
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-2863299679)
@EthanHeilman , thanks for the feedback.
I understand this combination should be discouraged.
My concern, perhaps not justified, was about this negative case not being documented/tested, thus wondering about a potential edge case.
Found out those tapscript test cases but I do not know if "not set" is exactly same as "set to false" (all those variables set to false by default?):
https://github.com/bitcoin/bitcoin/pull/29247/files#diff-ef0159635ee0fee8864cd9570a04337cabb63e9c49a9e96d7b746194e
...
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-2863299679)
@EthanHeilman , thanks for the feedback.
I understand this combination should be discouraged.
My concern, perhaps not justified, was about this negative case not being documented/tested, thus wondering about a potential edge case.
Found out those tapscript test cases but I do not know if "not set" is exactly same as "set to false" (all those variables set to false by default?):
https://github.com/bitcoin/bitcoin/pull/29247/files#diff-ef0159635ee0fee8864cd9570a04337cabb63e9c49a9e96d7b746194e
...
💬 theStack commented on pull request "test: refactor: negate signature-s using libsecp256k1":
(https://github.com/bitcoin/bitcoin/pull/32436#issuecomment-2863301427)
Thanks for the reviews! Applied your patch @laanwj and added a comment about the secp256k1 negation function usage.
> Conceptually, I'm in agreement with the replacement of custom negation code by using the one from secp256k1. Though I was a little surprised to note that git grep of this function doesn't lead to any actual usages of the function besides the tests & secp256k1_ec_privkey_negate function that too doesn't seem to be used elsewhere.
It would have been used for the BIP 151 (v2 t
...
(https://github.com/bitcoin/bitcoin/pull/32436#issuecomment-2863301427)
Thanks for the reviews! Applied your patch @laanwj and added a comment about the secp256k1 negation function usage.
> Conceptually, I'm in agreement with the replacement of custom negation code by using the one from secp256k1. Though I was a little surprised to note that git grep of this function doesn't lead to any actual usages of the function besides the tests & secp256k1_ec_privkey_negate function that too doesn't seem to be used elsewhere.
It would have been used for the BIP 151 (v2 t
...
🤔 furszy reviewed a pull request: "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor"
(https://github.com/bitcoin/bitcoin/pull/32344#pullrequestreview-2825377026)
It would be good to describe the steps to replicate the issue via RPC.
Including a range in a non-ranged descriptor import should cause the process to fail before reaching the wallet descriptor update function.
This step-by-step description would also allow us to convert the unit test into a functional test, which would be ideal, as it's simpler to maintain.
(https://github.com/bitcoin/bitcoin/pull/32344#pullrequestreview-2825377026)
It would be good to describe the steps to replicate the issue via RPC.
Including a range in a non-ranged descriptor import should cause the process to fail before reaching the wallet descriptor update function.
This step-by-step description would also allow us to convert the unit test into a functional test, which would be ideal, as it's simpler to maintain.