💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987507697)
Fixed on Transifex.
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987507697)
Fixed on Transifex.
💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987508337)
Removed on Transifex.
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987508337)
Removed on Transifex.
💬 Sjors commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2710949958)
Tested on macOS 15.3.1 that `cmake --build build --target deploy` still works. It's impossible (?) to test the missing condition, because /usr/bin/zip comes with the OS.
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2710949958)
Tested on macOS 15.3.1 that `cmake --build build --target deploy` still works. It's impossible (?) to test the missing condition, because /usr/bin/zip comes with the OS.
📝 marcofleon opened a pull request: "validation, fix: Use wtxid instead of txid in `CheckEphemeralSpends`"
(https://github.com/bitcoin/bitcoin/pull/32025)
This PR addresses a small bug in [`AcceptMultipleTransactions`](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/validation.cpp#L1598) where a txid was being inserted into a map that should only hold wtxids. `CheckEphemeralSpends` has an out parameter on failure that records that the child transaction did not spend the parent's dust. Instead of using the txid of this child, use its wtxid.
The second commit in this PR is a refactor of the `PackageMempoolAcc
...
(https://github.com/bitcoin/bitcoin/pull/32025)
This PR addresses a small bug in [`AcceptMultipleTransactions`](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/validation.cpp#L1598) where a txid was being inserted into a map that should only hold wtxids. `CheckEphemeralSpends` has an out parameter on failure that records that the child transaction did not spend the parent's dust. Instead of using the txid of this child, use its wtxid.
The second commit in this PR is a refactor of the `PackageMempoolAcc
...
💬 yancyribbens commented on pull request "tests: Improve stderr validation in test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/31966#issuecomment-2710957626)
Is there any reason the `TODO` was not removed on L159 with this commit?
(https://github.com/bitcoin/bitcoin/pull/31966#issuecomment-2710957626)
Is there any reason the `TODO` was not removed on L159 with this commit?
💬 marcofleon commented on pull request "validation, fix: Use wtxid instead of txid in `CheckEphemeralSpends`":
(https://github.com/bitcoin/bitcoin/pull/32025#issuecomment-2710962639)
note: I found this while working on the uint256 to txid/wtxid full refactor. I figured I would include a tiny part of that in this PR because it relates to the bug. If it's preferred to only have the fix be merged for branch off, then I can remove the second commit and include it as part of the txid type safety project later on.
(https://github.com/bitcoin/bitcoin/pull/32025#issuecomment-2710962639)
note: I found this while working on the uint256 to txid/wtxid full refactor. I figured I would include a tiny part of that in this PR because it relates to the bug. If it's preferred to only have the fix be merged for branch off, then I can remove the second commit and include it as part of the txid type safety project later on.
💬 rkrux commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2710976385)
I've started reviewing this PR and will leave a review soon in the coming days.
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2710976385)
I've started reviewing this PR and will leave a review soon in the coming days.
💬 Sjors commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2710977444)
Oh this code path isn't used at all when building natively? E.g. on macOS I can do `ZIP_EXECUTABLE=$HOME/temp/zip2 cmake --build build --target deploy` which doesn't fail.
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2710977444)
Oh this code path isn't used at all when building natively? E.g. on macOS I can do `ZIP_EXECUTABLE=$HOME/temp/zip2 cmake --build build --target deploy` which doesn't fail.
💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987530875)
No, it is not expected. Removed on Transifex.
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987530875)
No, it is not expected. Removed on Transifex.
💬 Sjors commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2711000319)
I'm not too worried about the size increase. A typical guix build eats 30GB on my Ubuntu VM (3 GB for the outputs).
> it makes it possible to check if mistakes have been made in the process at earlier steps.
Agreed.
It seems simple enough to backport, especially because we're not bumping the version.
I'll see if I can reproduce the new hashes.
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2711000319)
I'm not too worried about the size increase. A typical guix build eats 30GB on my Ubuntu VM (3 GB for the outputs).
> it makes it possible to check if mistakes have been made in the process at earlier steps.
Agreed.
It seems simple enough to backport, especially because we're not bumping the version.
I'll see if I can reproduce the new hashes.
💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2711014150)
Feedback from @achow101 has been addressed.
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2711014150)
Feedback from @achow101 has been addressed.
💬 Sjors commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2711029947)
One slight problem is that Apple no longer makes XCode 15.0 available for download. They do have Xcode 15.0.1: https://download.developer.apple.com/Developer_Tools/Xcode_15.0.1/Xcode_15.0.1.xip
I still have the original xip on one of my machines, I'll upload it somewhere for testing. But we should probably bump it.
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2711029947)
One slight problem is that Apple no longer makes XCode 15.0 available for download. They do have Xcode 15.0.1: https://download.developer.apple.com/Developer_Tools/Xcode_15.0.1/Xcode_15.0.1.xip
I still have the original xip on one of my machines, I'll upload it somewhere for testing. But we should probably bump it.
⚠️ janb84 opened an issue: "29.0 RC Testing Guide Feedback"
(https://github.com/bitcoin/bitcoin/issues/32026)
This issue is to discuss the [29.0 Release Candidate Testing Guide](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Candidate-Testing-Guide). If you have any feedback on the document, please leave a comment here.
Note: This is for feedback on the document, not on Bitcoin Core or on the 29.0 changes.
Thank you for taking a look at the guide and leaving your feedback.
(https://github.com/bitcoin/bitcoin/issues/32026)
This issue is to discuss the [29.0 Release Candidate Testing Guide](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Candidate-Testing-Guide). If you have any feedback on the document, please leave a comment here.
Note: This is for feedback on the document, not on Bitcoin Core or on the 29.0 changes.
Thank you for taking a look at the guide and leaving your feedback.
💬 pablomartin4btc commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1987563061)
But `CWallet::AddWalletDescriptor` could be called from other places (at the moment only tests and bench)? Doesn't matter?
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1987563061)
But `CWallet::AddWalletDescriptor` could be called from other places (at the moment only tests and bench)? Doesn't matter?
💬 pablomartin4btc commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1987567116)
Shouldn't return null as other validations like the previous one (`IsWalletFlagSet`)?
```suggestion
return nullptr;
```
Moreover, `DescriptorScriptPubKeyMan::UpdateWalletDescriptor` also calls `DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor`... could this call be removed?
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1987567116)
Shouldn't return null as other validations like the previous one (`IsWalletFlagSet`)?
```suggestion
return nullptr;
```
Moreover, `DescriptorScriptPubKeyMan::UpdateWalletDescriptor` also calls `DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor`... could this call be removed?
💬 dergoegge commented on pull request "validation, fix: Use wtxid instead of txid in `CheckEphemeralSpends`":
(https://github.com/bitcoin/bitcoin/pull/32025#issuecomment-2711052308)
Concept ACK
Awesome to see the txid types paying off!
(https://github.com/bitcoin/bitcoin/pull/32025#issuecomment-2711052308)
Concept ACK
Awesome to see the txid types paying off!
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1987579951)
https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1986316731
> I've always been a bit puzzled of this check. Is it really required? If so, why don't we need to check it on every iteration of the do loop further down? What if it is actually reached, will validation on that chain just always be stuck in an unrecoverable state? Wouldn't a fatal error be more appropriate then?
This is labeled as "Belt-and-suspenders check" so it should be safe to assume that it is not required. Sometim
...
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1987579951)
https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1986316731
> I've always been a bit puzzled of this check. Is it really required? If so, why don't we need to check it on every iteration of the do loop further down? What if it is actually reached, will validation on that chain just always be stuck in an unrecoverable state? Wouldn't a fatal error be more appropriate then?
This is labeled as "Belt-and-suspenders check" so it should be safe to assume that it is not required. Sometim
...
💬 hebasto commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2711068992)
> Oh this code path isn't used at all when building natively?
Correct.
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2711068992)
> Oh this code path isn't used at all when building natively?
Correct.
💬 dergoegge commented on pull request "test: get rid of redundant TODO tag in fuzz tests":
(https://github.com/bitcoin/bitcoin/pull/32024#issuecomment-2711071334)
Thanks for your contribution but `FuzzedDataProvider.h` is not maintained by us. This should be opened against llvm: https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h.
(https://github.com/bitcoin/bitcoin/pull/32024#issuecomment-2711071334)
Thanks for your contribution but `FuzzedDataProvider.h` is not maintained by us. This should be opened against llvm: https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h.
💬 laanwj commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2711075412)
Tested windows path without NSIS
```
$ ninja -j4
[224/224] Linking CXX executable src/test/test_bitcoin.exe (successful build)
$ ninja deploy
[1/1] cd /home/user/src/bitcoin-windows/build && /usr/bin/cmake -E echo && /usr/bin/cm... -E echo "Then re-run cmake to regenerate the build system." && /usr/bin/cmake -E echo
Error: NSIS not found.
Please install NSIS and/or ensure that its executable is accessible to the find_program() command—
for example, by setting the MAKENSIS_EXECUTABLE va
...
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2711075412)
Tested windows path without NSIS
```
$ ninja -j4
[224/224] Linking CXX executable src/test/test_bitcoin.exe (successful build)
$ ninja deploy
[1/1] cd /home/user/src/bitcoin-windows/build && /usr/bin/cmake -E echo && /usr/bin/cm... -E echo "Then re-run cmake to regenerate the build system." && /usr/bin/cmake -E echo
Error: NSIS not found.
Please install NSIS and/or ensure that its executable is accessible to the find_program() command—
for example, by setting the MAKENSIS_EXECUTABLE va
...