💬 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.
✅ fanquake closed an issue: "32-bit Linux: build flags lost with depends & overriden CC(X)"
(https://github.com/bitcoin/bitcoin/issues/28096)
(https://github.com/bitcoin/bitcoin/issues/28096)
💬 EthanHeilman commented on pull request "CAT in Tapscript (BIP-347)":
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-2863344633)
@AlexSQY I see your point. Let me see if there is anything that can be done to ensure it is never the case that `OP_CAT = False && DISCOURAGE_OP_CAT = False` since that should never happen outside of tests.
The behavior is what you see in that test is the expected behavior for `DISCOURAGE_OP_CAT = False` and `OP_CAT = False`,
since `OP_CAT = False` the CAT opcode is an OP_SUCCESS so it returns success (`OK`).
@0xBEEFCAF3 What do you think?
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-2863344633)
@AlexSQY I see your point. Let me see if there is anything that can be done to ensure it is never the case that `OP_CAT = False && DISCOURAGE_OP_CAT = False` since that should never happen outside of tests.
The behavior is what you see in that test is the expected behavior for `DISCOURAGE_OP_CAT = False` and `OP_CAT = False`,
since `OP_CAT = False` the CAT opcode is an OP_SUCCESS so it returns success (`OK`).
@0xBEEFCAF3 What do you think?
💬 marcofleon commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2079888041)
I think it would be better to consume from a range here and below. `MAX_SCRIPT_SIZE` is large (10000) so the fuzzer ends up consuming the remaining bytes in the data provider at this line.
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2079888041)
I think it would be better to consume from a range here and below. `MAX_SCRIPT_SIZE` is large (10000) so the fuzzer ends up consuming the remaining bytes in the data provider at this line.
💬 stickies-v commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2863362683)
re-ACK 0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1, no changes since 65fcfbb2b38bef20a58daa6c828c51890180611d except rebase.
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2863362683)
re-ACK 0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1, no changes since 65fcfbb2b38bef20a58daa6c828c51890180611d except rebase.
👍 ryanofsky approved a pull request: "cmake: Respect user-provided configuration-specific flags"
(https://github.com/bitcoin/bitcoin/pull/32356#pullrequestreview-2825385905)
Code review ACK edde96376a2961dec3730331b3d171ddf972589f
(https://github.com/bitcoin/bitcoin/pull/32356#pullrequestreview-2825385905)
Code review ACK edde96376a2961dec3730331b3d171ddf972589f