Bitcoin Core Github
42 subscribers
126K links
Download Telegram
💬 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.
👍 TheCharlatan approved a pull request: "[26.x] Changes for rc3"
(https://github.com/bitcoin/bitcoin/pull/28872#pullrequestreview-1745009742)
ACK 2f86d3053314c382dfce5cf314e98811ed433859
💬 pinheadmz commented on pull request "test: fix `AddNode` unit test failure on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/28891#issuecomment-1823254133)
> cc @pinheadmz who is apparently also running an OpenBSD server, as far as I remember :-)

Hi, post merge code review ACK.

I have a FreeBSD container on google cloud I just fire up when needed.

The bug you're fixing here is apparently not an issue on my OS:
`FreeBSD freebsd-13-1 13.2-RELEASE-p4 FreeBSD 13.2-RELEASE-p4 GENERIC amd64`

The bitcoin unit test passes without this patch and `ping 127.1` also works fine.
🚀 fanquake merged a pull request: "[26.x] Changes for rc3"
(https://github.com/bitcoin/bitcoin/pull/28872)
fanquake closed a pull request: "Create master"
(https://github.com/bitcoin/bitcoin/pull/28593)