💬 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
💬 ryanofsky commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2079862300)
In commit "cmake: Respect user-provided configuration-specific flags" (edde96376a2961dec3730331b3d171ddf972589f)
Would be good for the comment here to note that this needs to be called before the project() command otherwise the list will include variables not actually set by the user. (I wouldn't have known that unless I saw the discussion at https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2075396465)
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2079862300)
In commit "cmake: Respect user-provided configuration-specific flags" (edde96376a2961dec3730331b3d171ddf972589f)
Would be good for the comment here to note that this needs to be called before the project() command otherwise the list will include variables not actually set by the user. (I wouldn't have known that unless I saw the discussion at https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2075396465)
👍 laanwj approved a pull request: "test: refactor: negate signature-s using libsecp256k1"
(https://github.com/bitcoin/bitcoin/pull/32436#pullrequestreview-2825454444)
Code review ACK 1ee698fde2e8652d8eb083749edb8029c003b8db
(https://github.com/bitcoin/bitcoin/pull/32436#pullrequestreview-2825454444)
Code review ACK 1ee698fde2e8652d8eb083749edb8029c003b8db
👍 laanwj approved a pull request: "contrib: remove bdb exception from FORTIFY check"
(https://github.com/bitcoin/bitcoin/pull/32448#pullrequestreview-2825462550)
Code review ACK f9dfe8d5e0d3f628659702ab781b7919505c829f
(https://github.com/bitcoin/bitcoin/pull/32448#pullrequestreview-2825462550)
Code review ACK f9dfe8d5e0d3f628659702ab781b7919505c829f
🤔 ismaelsadeeq reviewed a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2825308443)
ACK 0ac19a98f329fe8952f394509a7a29775ab805b0 with a nit and another suggestion
The size limit check above the sigops check also underestimates the block size because it does not include the witness.
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2825308443)
ACK 0ac19a98f329fe8952f394509a7a29775ab805b0 with a nit and another suggestion
The size limit check above the sigops check also underestimates the block size because it does not include the witness.