📝 fanquake locked a pull request: "Update compat.h"
(https://github.com/bitcoin/bitcoin/pull/29118)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/29118)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 ben-arnao commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1863993219)
Just my two cents.. if you look at this from a perspective of a bad actor paying to hurt BTC network, as long as there is a cost required to keep up the attack what is the issue? Trying to filter txs that we think are not legitimate seems like a risky road to go down.
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1863993219)
Just my two cents.. if you look at this from a perspective of a bad actor paying to hurt BTC network, as long as there is a cost required to keep up the attack what is the issue? Trying to filter txs that we think are not legitimate seems like a risky road to go down.
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1863995929)
Code review ACK db6b61e9e7635c9cb97d73fd44b9e7266b8eef51
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1863995929)
Code review ACK db6b61e9e7635c9cb97d73fd44b9e7266b8eef51
💬 kristapsk commented on pull request "include verbose debug messages in testmempoolaccept reject-reason":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-1864000603)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-1864000603)
Concept ACK
🤔 S3RK reviewed a pull request: "wallet, rpc: `FundTransaction` refactor"
(https://github.com/bitcoin/bitcoin/pull/28560#pullrequestreview-1790386436)
I'm still trying to fully grasp the existing code (which is a big mess), so bear with me if I don't make sense. Left some comment/questions based on my current understanding.
(https://github.com/bitcoin/bitcoin/pull/28560#pullrequestreview-1790386436)
I'm still trying to fully grasp the existing code (which is a big mess), so bear with me if I don't make sense. Left some comment/questions based on my current understanding.
💬 S3RK commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432414337)
Same comment as for other `FundTransaction`. I think we should get rid of ambiguity in input parameters and don't pass `tx`. In this case it's even more confusing, because `tx.vout` is actually still checked.
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432414337)
Same comment as for other `FundTransaction`. I think we should get rid of ambiguity in input parameters and don't pass `tx`. In this case it's even more confusing, because `tx.vout` is actually still checked.
💬 S3RK commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432396784)
`has_data` never set to `true`
Also no tests fail, so it seems there is no test coverage for that
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432396784)
`has_data` never set to `true`
Also no tests fail, so it seems there is no test coverage for that
💬 S3RK commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432411126)
Not a huge fan of this change. Now if we pass tx.vout they'll be ignored.
I'd prefer if we pass `txin` and `recipients` and don't pass `tx` at all. That would be a clearer interface in my opinion.
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432411126)
Not a huge fan of this change. Now if we pass tx.vout they'll be ignored.
I'd prefer if we pass `txin` and `recipients` and don't pass `tx` at all. That would be a clearer interface in my opinion.
💬 S3RK commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432392999)
Let's not pass all the options as we need only one of them.
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432392999)
Let's not pass all the options as we need only one of them.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1864100266)
Finally submitted the optimized code.
I also [tried caching](https://github.com/naumenkogs/bitcoin/commit/ae19891dfb588daefcda475a860464587f0480dc), but the speed went _down_ 10x (either map or unordered_map). Perhaps @sipa @mzumsande have interest in looking what could be the issue.
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1864100266)
Finally submitted the optimized code.
I also [tried caching](https://github.com/naumenkogs/bitcoin/commit/ae19891dfb588daefcda475a860464587f0480dc), but the speed went _down_ 10x (either map or unordered_map). Perhaps @sipa @mzumsande have interest in looking what could be the issue.
💬 fanquake commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-1864156198)
> Looks like this also fails on macOS on GHA.
Pretty sure that job should be rerun, because it's failing in the brew install stage.
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-1864156198)
> Looks like this also fails on macOS on GHA.
Pretty sure that job should be rerun, because it's failing in the brew install stage.
💬 fanquake commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-1864170372)
> Does anyone know what version(s) of clang can be expected to be available on macOS normally?
GIven our minimum supported macOS (11.x), you could run anything from Xcode 12.5+ onwards, which is roughly LLVM/Clang 12+. Although I'd expect most users compiling from source on macOS, would be using a much more recent Xcode, so likely Clang ~14+ at least.
Also, anyone should be able to install and run the latest LLVM/Clang via brew.
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-1864170372)
> Does anyone know what version(s) of clang can be expected to be available on macOS normally?
GIven our minimum supported macOS (11.x), you could run anything from Xcode 12.5+ onwards, which is roughly LLVM/Clang 12+. Although I'd expect most users compiling from source on macOS, would be using a much more recent Xcode, so likely Clang ~14+ at least.
Also, anyone should be able to install and run the latest LLVM/Clang via brew.
💬 VojtechMyslivec commented on issue "gettransaction details does not include send to myself balance changes for imported addresses":
(https://github.com/bitcoin/bitcoin/issues/28555#issuecomment-1864179337)
Is there anything more I can provide to investigate this problem?
I would label this with ~bug, ~descriptors and ~wallet
Thanks
(https://github.com/bitcoin/bitcoin/issues/28555#issuecomment-1864179337)
Is there anything more I can provide to investigate this problem?
I would label this with ~bug, ~descriptors and ~wallet
Thanks
💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1864179391)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1864179391)
Concept ACK
💬 maflcko commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1864192035)
Though, on GHA it says XCode 14.3, see https://github.com/bitcoin/bitcoin/actions/runs/7171999335/job/19528203033?pr=28687#step:3:14
```
clang --version
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: x86_64-apple-darwin22.6.0
Thread model: posix
InstalledDir: /Applications/Xcode_14.3.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1864192035)
Though, on GHA it says XCode 14.3, see https://github.com/bitcoin/bitcoin/actions/runs/7171999335/job/19528203033?pr=28687#step:3:14
```
clang --version
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: x86_64-apple-darwin22.6.0
Thread model: posix
InstalledDir: /Applications/Xcode_14.3.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
💬 fanquake commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1864200442)
> Though, on GHA it says XCode 14.3
According to https://developer.apple.com/xcode/cpp/#c++20, "Ranges" have been available on macOS since Xcode 14.3.
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1864200442)
> Though, on GHA it says XCode 14.3
According to https://developer.apple.com/xcode/cpp/#c++20, "Ranges" have been available on macOS since Xcode 14.3.
💬 fanquake commented on pull request "build: Bump guix time-machine to unlock riscv64 metal":
(https://github.com/bitcoin/bitcoin/pull/29078#issuecomment-1864204382)
> Looks like this fails with:
Given that the problem here is compiling cctools on RISC-V, I think this will ultimately be solved by switching to LLVM/LLD (#21778).
(https://github.com/bitcoin/bitcoin/pull/29078#issuecomment-1864204382)
> Looks like this fails with:
Given that the problem here is compiling cctools on RISC-V, I think this will ultimately be solved by switching to LLVM/LLD (#21778).
💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1864250140)
re-ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c 🌚
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK e60fc7d5d34f23cccbff
...
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1864250140)
re-ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c 🌚
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK e60fc7d5d34f23cccbff
...
⚠️ ManuelSchneid3r opened an issue: "Weird focus rect displayed on inital sync"
(https://github.com/bitcoin-core/gui/issues/783)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
<img width="891" alt="Bildschirmfoto 2023-12-20 um 11 41 09" src="https://github.com/bitcoin-core/gui/assets/3252315/74c17ab2-460d-4eba-8151-3f19c35b71a7">
### Expected behaviour
<img width="866" alt="Bildschirmfoto 2023-12-20 um 11 43 55" src="https://github.com/bitcoin-core/gui/assets/3252315/7748c491-e0be-4f34-bb4e-c6e39f7c1244">
### Steps to reproduce
Start Bitcoin-Qt
### Rele
...
(https://github.com/bitcoin-core/gui/issues/783)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
<img width="891" alt="Bildschirmfoto 2023-12-20 um 11 41 09" src="https://github.com/bitcoin-core/gui/assets/3252315/74c17ab2-460d-4eba-8151-3f19c35b71a7">
### Expected behaviour
<img width="866" alt="Bildschirmfoto 2023-12-20 um 11 43 55" src="https://github.com/bitcoin-core/gui/assets/3252315/7748c491-e0be-4f34-bb4e-c6e39f7c1244">
### Steps to reproduce
Start Bitcoin-Qt
### Rele
...
💬 ManuelSchneid3r commented on issue "Weird focus rect displayed on inital sync":
(https://github.com/bitcoin-core/gui/issues/783#issuecomment-1864257245)
Just updated to 26. still there.
(https://github.com/bitcoin-core/gui/issues/783#issuecomment-1864257245)
Just updated to 26. still there.