📝 stevenroose opened a pull request: "Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes"
(https://github.com/bitcoin/bitcoin/pull/29050)
Implementation of OP_TXHASH and OP_CHECKTXHASHVERIFY, as per the [draft BIP](https://github.com/bitcoin/bips/pull/1500).
This MR includes a test using the test vectors generated from the reference implementation in the BIP.
The implementation utilizes a caching strategy that hopefully alleviates concerns around resource usage like quadratic hashing.
(https://github.com/bitcoin/bitcoin/pull/29050)
Implementation of OP_TXHASH and OP_CHECKTXHASHVERIFY, as per the [draft BIP](https://github.com/bitcoin/bips/pull/1500).
This MR includes a test using the test vectors generated from the reference implementation in the BIP.
The implementation utilizes a caching strategy that hopefully alleviates concerns around resource usage like quadratic hashing.
💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#issuecomment-1849599335)
Code review ACK 0295b44c257fe23f1ad344aff11372d67864f0db
Still not a huge fan of 596642c5a9 and I have same concerns as josibake. I prefer explicit defitino of external inputs rather than indirectly through `SetTxOut`. Though I don't want this to block the PR.
Also it seems two commits were squashed "Explicitly preserve scriptSig and scriptWitness in CreateTransaction" and "Preserve preset input order". Was that intentional?
(https://github.com/bitcoin/bitcoin/pull/25273#issuecomment-1849599335)
Code review ACK 0295b44c257fe23f1ad344aff11372d67864f0db
Still not a huge fan of 596642c5a9 and I have same concerns as josibake. I prefer explicit defitino of external inputs rather than indirectly through `SetTxOut`. Though I don't want this to block the PR.
Also it seems two commits were squashed "Explicitly preserve scriptSig and scriptWitness in CreateTransaction" and "Preserve preset input order". Was that intentional?
💬 S3RK commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#issuecomment-1849606079)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/28920#issuecomment-1849606079)
Approach ACK
💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#issuecomment-1849613826)
> Still not a huge fan of https://github.com/bitcoin/bitcoin/commit/596642c5a9f52dda2599b0bde424366bb22b3c6e and I have same concerns as josibake
My concerns were addressed by https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420969629. I think the approach in https://github.com/bitcoin/bitcoin/commit/596642c5a9f52dda2599b0bde424366bb22b3c6e is much simpler and as the comment points out, the worst-case scenario if inputs are labeled incorrectly is overestimating the fee by 1 byte.
(https://github.com/bitcoin/bitcoin/pull/25273#issuecomment-1849613826)
> Still not a huge fan of https://github.com/bitcoin/bitcoin/commit/596642c5a9f52dda2599b0bde424366bb22b3c6e and I have same concerns as josibake
My concerns were addressed by https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420969629. I think the approach in https://github.com/bitcoin/bitcoin/commit/596642c5a9f52dda2599b0bde424366bb22b3c6e is much simpler and as the comment points out, the worst-case scenario if inputs are labeled incorrectly is overestimating the fee by 1 byte.
⚠️ nymkappa opened an issue: "[build] cannot build tests using v26.0"
(https://github.com/bitcoin/bitcoin/issues/29051)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I'm trying to upgrade to core v26.0 on macOS (M1 processor) but I'm getting the following error when building from sources:
```
CXX test/test_bitcoin-base58_tests.o
test/base58_tests.cpp:26:22: error: no matching function for call to 'read_json'
UniValue tests = read_json(json_tests::base58_encode_decode);
^~~~~~~~~
./test/util/json.h:12:10: note: ca
...
(https://github.com/bitcoin/bitcoin/issues/29051)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I'm trying to upgrade to core v26.0 on macOS (M1 processor) but I'm getting the following error when building from sources:
```
CXX test/test_bitcoin-base58_tests.o
test/base58_tests.cpp:26:22: error: no matching function for call to 'read_json'
UniValue tests = read_json(json_tests::base58_encode_decode);
^~~~~~~~~
./test/util/json.h:12:10: note: ca
...
💬 willcl-ark commented on issue "[build] cannot build tests using v26.0":
(https://github.com/bitcoin/bitcoin/issues/29051#issuecomment-1849617387)
Have you run `make clean` since updating your local repo to v26.0?
(https://github.com/bitcoin/bitcoin/issues/29051#issuecomment-1849617387)
Have you run `make clean` since updating your local repo to v26.0?
🚀 fanquake merged a pull request: "Add a note to msvc readme re building Qt for Bitcoin Core."
(https://github.com/bitcoin/bitcoin/pull/29048)
(https://github.com/bitcoin/bitcoin/pull/29048)
💬 fanquake commented on pull request "ci: Use Ubuntu 24.04 Noble for asan,tsan,tidy,fuzz":
(https://github.com/bitcoin/bitcoin/pull/28992#issuecomment-1849657966)
Looks like something similar (for LLVM 18) is being worked around upstream, by just creating the missing file: https://github.com/include-what-you-use/include-what-you-use/pull/1350.
(https://github.com/bitcoin/bitcoin/pull/28992#issuecomment-1849657966)
Looks like something similar (for LLVM 18) is being worked around upstream, by just creating the missing file: https://github.com/include-what-you-use/include-what-you-use/pull/1350.
💬 naumenkogs commented on pull request "fuzz: p2p: Detect peer deadlocks":
(https://github.com/bitcoin/bitcoin/pull/29009#issuecomment-1849658906)
ACK 9f265d88253ed464413dea5614fa13dea0d8cfd5
(https://github.com/bitcoin/bitcoin/pull/29009#issuecomment-1849658906)
ACK 9f265d88253ed464413dea5614fa13dea0d8cfd5
🚀 fanquake merged a pull request: "msvc: Optimize "Release" builds"
(https://github.com/bitcoin/bitcoin/pull/29045)
(https://github.com/bitcoin/bitcoin/pull/29045)
💬 naumenkogs commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#issuecomment-1849667277)
Concept ACK. Looks cleaner. Will look in more detail when you undraft it.
(https://github.com/bitcoin/bitcoin/pull/29039#issuecomment-1849667277)
Concept ACK. Looks cleaner. Will look in more detail when you undraft it.
🚀 fanquake merged a pull request: "test: fix `addnode` functional test failure on OpenBSD"
(https://github.com/bitcoin/bitcoin/pull/29035)
(https://github.com/bitcoin/bitcoin/pull/29035)
💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1849679829)
> Does it happen on the master branch?
@hebasto, yes, see https://github.com/bitcoin/bitcoin/pull/28774
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1849679829)
> Does it happen on the master branch?
@hebasto, yes, see https://github.com/bitcoin/bitcoin/pull/28774
✅ nymkappa closed an issue: "[build] cannot build tests using v26.0"
(https://github.com/bitcoin/bitcoin/issues/29051)
(https://github.com/bitcoin/bitcoin/issues/29051)
💬 nymkappa commented on issue "[build] cannot build tests using v26.0":
(https://github.com/bitcoin/bitcoin/issues/29051#issuecomment-1849683631)
> Have you run `make clean` since updating your local repo to v26.0?
I did not. I was not aware I should do this when upgrading. Thanks for the tip. It builds and run fine now on v26.0 @ 44d8b13c81e5276eb610c99f227a4d090cc532f6. All tests also passed. Thank you!
(https://github.com/bitcoin/bitcoin/issues/29051#issuecomment-1849683631)
> Have you run `make clean` since updating your local repo to v26.0?
I did not. I was not aware I should do this when upgrading. Thanks for the tip. It builds and run fine now on v26.0 @ 44d8b13c81e5276eb610c99f227a4d090cc532f6. All tests also passed. Thank you!
💬 fanquake commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422215535)
Depending on what happens here, given we only use `CountBits` in two places, if this is going to basically become a std lib call, we could just use it directly where needed, and remove our `CountBits` unit/fuzz tests, so that we aren't nearly unit testing / fuzzing the standard library.
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422215535)
Depending on what happens here, given we only use `CountBits` in two places, if this is going to basically become a std lib call, we could just use it directly where needed, and remove our `CountBits` unit/fuzz tests, so that we aren't nearly unit testing / fuzzing the standard library.
✅ willcl-ark closed an issue: "Bitcoin Core - Transaction without permition"
(https://github.com/bitcoin/bitcoin/issues/29049)
(https://github.com/bitcoin/bitcoin/issues/29049)
💬 willcl-ark commented on issue "Bitcoin Core - Transaction without permition":
(https://github.com/bitcoin/bitcoin/issues/29049#issuecomment-1849701776)
Hello @hugomenezes85 .
I'm sorry to hear about your unauthorised transaction. If this has really taken place without your knowledge it could indicate that someone has obtained a copy of your wallet file, private key(s) or other senstive key material in order to make this transaction. This would also correlate with Bitcoin Core not needing to open the wallet for this to happen -- Bitcoin Core did not make the transaction, only picked up that it had occured from the blockchain. I'd encourage yo
...
(https://github.com/bitcoin/bitcoin/issues/29049#issuecomment-1849701776)
Hello @hugomenezes85 .
I'm sorry to hear about your unauthorised transaction. If this has really taken place without your knowledge it could indicate that someone has obtained a copy of your wallet file, private key(s) or other senstive key material in order to make this transaction. This would also correlate with Bitcoin Core not needing to open the wallet for this to happen -- Bitcoin Core did not make the transaction, only picked up that it had occured from the blockchain. I'd encourage yo
...
🤔 vasild reviewed a pull request: "refactor: Remove pre-C++20 code, fs::path cleanup"
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1774737399)
Almost ACK 0d66eea93f1e115b2e9e00ee2e89cd967f826d22, modulo the comment below about the `u8string()` method.
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1774737399)
Almost ACK 0d66eea93f1e115b2e9e00ee2e89cd967f826d22, modulo the comment below about the `u8string()` method.
💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422242907)
What about the comment that says "This method can be removed after switching to C++20"? I think indeed this method should be removed entirely.
In C++17 its prototype, according to the spec is `std::string u8string() const` which matches ours and is ok. But in C++20 it is: `std::u8string u8string() const` and it is confusing to have a mismatch. I would better drop this method like the comment says and then fix problems (if any) that arise due to that.
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422242907)
What about the comment that says "This method can be removed after switching to C++20"? I think indeed this method should be removed entirely.
In C++17 its prototype, according to the spec is `std::string u8string() const` which matches ours and is ok. But in C++20 it is: `std::u8string u8string() const` and it is confusing to have a mismatch. I would better drop this method like the comment says and then fix problems (if any) that arise due to that.