Bitcoin Core Github
42 subscribers
126K links
Download Telegram
🤔 pablomartin4btc reviewed a pull request: "getrawtransaction implementation"
(https://github.com/bitcoin-core/gui/pull/777#pullrequestreview-1744793504)
Concept ACK

As @luke-jr perhaps we can create a separate menu item like "Tools" or "Utils" with this feature in it (and would be more suitable if we need to add for example other features as @willcl-ark [mentioned](https://github.com/bitcoin-core/gui/pull/777#issuecomment-1810068057)).

Light CR.
💬 pablomartin4btc commented on pull request "getrawtransaction implementation":
(https://github.com/bitcoin-core/gui/pull/777#discussion_r1402321260)
Since you are here (refactoring commit 10b3cd129e1fa8ece229a255d6fff89f419751c0) I'd extract all these settings into a function like "`initializeAboutModeWindow`" or something like that, same for the other modes, making the code a bit more maintainable and easier to follow.
💬 ismaelsadeeq commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#issuecomment-1823047081)
Concept ACK
💬 fanquake commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#issuecomment-1823088898)
Picked up in #28926.
💬 fanquake commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1823090489)
```bash
clang-tidy-17 -p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/rpc/net.cpp
rpc/net.cpp:726:30: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
726 | path.push_back("totals");
| ^~~~~~~~~~
| emplace_back(
```
💬 fanquake commented on pull request "[26.x] Changes for rc3":
(https://github.com/bitcoin/bitcoin/pull/28872#discussion_r1402376062)
No worries, fixed up.
🚀 fanquake merged a pull request: "lint: Report all lint errors instead of early exit"
(https://github.com/bitcoin/bitcoin/pull/28862)
💬 brunoerg commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1823155627)
Concept ACK
💬 fanquake commented on pull request "build: Fix regression in "ARMv8 CRC32 intrinsics" test":
(https://github.com/bitcoin/bitcoin/pull/28919#issuecomment-1823164420)
> The vmull_p64 is a part of the Crypto extensions from the ACLE. They are optional extensions, so they get enabled with a +crypto for architecture flags.

I guess this isn't true for all aarch64, as the checks currently work fine, for example, on Apple arm64 (M1):
```bash
checking for AVX2 intrinsics... no
checking for x86 SHA-NI intrinsics... no
checking whether C++ compiler accepts -march=armv8-a+crc... yes
checking whether C++ compiler accepts -march=armv8-a+crypto... yes
checking fo
...
👍 hebasto approved a pull request: "[26.x] Changes for rc3"
(https://github.com/bitcoin/bitcoin/pull/28872#pullrequestreview-1744950215)
re-ACK f868852d3d7321aa522d6b168f33bef1e5eba2dc.
💬 hebasto commented on pull request "build: Fix regression in "ARMv8 CRC32 intrinsics" test":
(https://github.com/bitcoin/bitcoin/pull/28919#issuecomment-1823172024)
> > The vmull_p64 is a part of the Crypto extensions from the ACLE. They are optional extensions, so they get enabled with a +crypto for architecture flags.
>
> I guess this isn't true for all aarch64, as the checks currently work fine, for example, on Apple arm64 (M1)

Right. Mind suggesting a more precise wording?
💬 fanquake commented on pull request "build: Fix regression in "ARMv8 CRC32 intrinsics" test":
(https://github.com/bitcoin/bitcoin/pull/28919#issuecomment-1823179632)
> I guess this isn't true for all aarch64, as the checks currently work fine, for example, on Apple arm64 (M1):

Actually, I think this is just an Apple special case, and because Apple Clang just enables `+crypto` and +`crc` as part of the default compile flags.
1
fanquake closed an issue: "win: non-static libssp in libbitcoinconsensus DLL"
(https://github.com/bitcoin/bitcoin/issues/28104)
1
🚀 fanquake merged a pull request: "build: Windows SSP roundup"
(https://github.com/bitcoin/bitcoin/pull/28461)
🚀 fanquake merged a pull request: "build: Fix regression in "ARMv8 CRC32 intrinsics" test"
(https://github.com/bitcoin/bitcoin/pull/28919)
💬 hebasto commented on pull request "build: Fix regression in "ARMv8 CRC32 intrinsics" test":
(https://github.com/bitcoin/bitcoin/pull/28919#issuecomment-1823187990)
Is it worth backporting to 26.x?
💬 fanquake commented on pull request "build: Fix regression in "ARMv8 CRC32 intrinsics" test":
(https://github.com/bitcoin/bitcoin/pull/28919#issuecomment-1823188307)
Backported in #28872.
👍 hebasto approved a pull request: "[26.x] Changes for rc3"
(https://github.com/bitcoin/bitcoin/pull/28872#pullrequestreview-1744989681)
re-ACK 2f86d3053314c382dfce5cf314e98811ed433859, only https://github.com/bitcoin/bitcoin/pull/28919 backported since my [recent](https://github.com/bitcoin/bitcoin/pull/28872#pullrequestreview-1744950215) review.
🤔 pablomartin4btc reviewed a pull request: "Use Txid in COutpoint"
(https://github.com/bitcoin/bitcoin/pull/28922#pullrequestreview-1744990371)
Concept ACK

Light CR.
💬 pablomartin4btc commented on pull request "Use Txid in COutpoint":
(https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1402451595)
Are you keeping this and removing it in a follow-up?

https://github.com/bitcoin/bitcoin/blob/9e58c5bcd96e7ff2062274868814ccae0626589e/src/util/transaction_identifier.h#L53-L60

Same for the comparisons above those lines (#L18 `// TODO`).
📝 brunoerg opened a pull request: "test: add coverage for bech32m in `wallet_keypool_topup`"
(https://github.com/bitcoin/bitcoin/pull/28928)
0dcac51049cdd924a50d62629757effc8d542046 added coverage for all keypool addresses types in `wallet_keypool_topup` (4y agp). Now we have bech23m, so this PR adds it.