💬 vasild commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1641806637)
Here is a patch on top of this PR that should address all concerns above about thread safetyness.
<details>
<summary>[patch] thread safe</summary>
```diff
diff --git i/src/net.cpp w/src/net.cpp
index c2160e945f..058f615dd5 100644
--- i/src/net.cpp
+++ w/src/net.cpp
@@ -886,12 +886,14 @@ size_t CConnman::SocketSendData(CNode& node) const
* to forge. In order to partition a node the attacker must be
* simultaneously better at all of them than honest peers.
* If we find
...
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1641806637)
Here is a patch on top of this PR that should address all concerns above about thread safetyness.
<details>
<summary>[patch] thread safe</summary>
```diff
diff --git i/src/net.cpp w/src/net.cpp
index c2160e945f..058f615dd5 100644
--- i/src/net.cpp
+++ w/src/net.cpp
@@ -886,12 +886,14 @@ size_t CConnman::SocketSendData(CNode& node) const
* to forge. In order to partition a node the attacker must be
* simultaneously better at all of them than honest peers.
* If we find
...
💬 MarcoFalke commented on pull request "build: promote -Wunused-member-function to -Wunused":
(https://github.com/bitcoin/bitcoin/pull/28102#issuecomment-1641808324)
For gcc this is already in Wall. Not sure for clang.
Not sure if related: https://cirrus-ci.com/task/6059497324544000?logs=ci#L2338
(https://github.com/bitcoin/bitcoin/pull/28102#issuecomment-1641808324)
For gcc this is already in Wall. Not sure for clang.
Not sure if related: https://cirrus-ci.com/task/6059497324544000?logs=ci#L2338
💬 hebasto commented on pull request "build: promote -Wunused-member-function to -Wunused":
(https://github.com/bitcoin/bitcoin/pull/28102#issuecomment-1641810745)
For clang, [`-Wunused`](https://clang.llvm.org/docs/DiagnosticsReference.html#wunused) does not control `-Wunused-member-function`, no?
(https://github.com/bitcoin/bitcoin/pull/28102#issuecomment-1641810745)
For clang, [`-Wunused`](https://clang.llvm.org/docs/DiagnosticsReference.html#wunused) does not control `-Wunused-member-function`, no?
💬 hebasto commented on pull request "depends: xcb-proto 1.15.2":
(https://github.com/bitcoin/bitcoin/pull/28097#issuecomment-1641813666)
Guix builds:
```
6d1725c9346bdf04e1eeec2deffda90957bd081700ba896834a143756410fb0e guix-build-7cb88c8b4672/output/aarch64-linux-gnu/SHA256SUMS.part
34486c39daea8a3ce8d92e9c1501f26e5f5e300d9612a25bd56c48ab56353329 guix-build-7cb88c8b4672/output/aarch64-linux-gnu/bitcoin-7cb88c8b4672-aarch64-linux-gnu-debug.tar.gz
cb0080d75d0fc4fc5c7fc022b5771470c60609c35836e7b66c4f479a2edcd098 guix-build-7cb88c8b4672/output/aarch64-linux-gnu/bitcoin-7cb88c8b4672-aarch64-linux-gnu.tar.gz
395c332f83206125819
...
(https://github.com/bitcoin/bitcoin/pull/28097#issuecomment-1641813666)
Guix builds:
```
6d1725c9346bdf04e1eeec2deffda90957bd081700ba896834a143756410fb0e guix-build-7cb88c8b4672/output/aarch64-linux-gnu/SHA256SUMS.part
34486c39daea8a3ce8d92e9c1501f26e5f5e300d9612a25bd56c48ab56353329 guix-build-7cb88c8b4672/output/aarch64-linux-gnu/bitcoin-7cb88c8b4672-aarch64-linux-gnu-debug.tar.gz
cb0080d75d0fc4fc5c7fc022b5771470c60609c35836e7b66c4f479a2edcd098 guix-build-7cb88c8b4672/output/aarch64-linux-gnu/bitcoin-7cb88c8b4672-aarch64-linux-gnu.tar.gz
395c332f83206125819
...
💬 fanquake commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1641813690)
> I don't care about updating the existing code,
To be clear, I agree, and am still a NACK to any code changes here.
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1641813690)
> I don't care about updating the existing code,
To be clear, I agree, and am still a NACK to any code changes here.
💬 fanquake commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1641828054)
> I should report this upstream as per https://geti2p.net/en/faq#bug.
Great. Can you link to the issue once it's reported upstream, as it'd be good to know which (if any) of the changes here are required just to work around buggy router software.
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1641828054)
> I should report this upstream as per https://geti2p.net/en/faq#bug.
Great. Can you link to the issue once it's reported upstream, as it'd be good to know which (if any) of the changes here are required just to work around buggy router software.
🤔 hebasto reviewed a pull request: "validation: use noexcept instead of deprecated throw()"
(https://github.com/bitcoin/bitcoin/pull/28090#pullrequestreview-1536785324)
Post-merge ACK 047daad4f59942488163c6be8516a69291646294.
(https://github.com/bitcoin/bitcoin/pull/28090#pullrequestreview-1536785324)
Post-merge ACK 047daad4f59942488163c6be8516a69291646294.
💬 fanquake commented on pull request "build: promote -Wunused-member-function to -Wunused":
(https://github.com/bitcoin/bitcoin/pull/28102#issuecomment-1641843137)
> Not sure if related: https://cirrus-ci.com/task/6059497324544000?logs=ci#L2338
I thought we'd cleaned these up at some point, can be left as-is in any case.
(https://github.com/bitcoin/bitcoin/pull/28102#issuecomment-1641843137)
> Not sure if related: https://cirrus-ci.com/task/6059497324544000?logs=ci#L2338
I thought we'd cleaned these up at some point, can be left as-is in any case.
✅ fanquake closed a pull request: "build: promote -Wunused-member-function to -Wunused"
(https://github.com/bitcoin/bitcoin/pull/28102)
(https://github.com/bitcoin/bitcoin/pull/28102)
✅ fanquake closed an issue: "libsecp CI failure [no wallet, libbitcoinkernel] [focal]"
(https://github.com/bitcoin/bitcoin/issues/28079)
(https://github.com/bitcoin/bitcoin/issues/28079)
🚀 fanquake merged a pull request: "subtree: update libsecp256k1 to latest master"
(https://github.com/bitcoin/bitcoin/pull/28093)
(https://github.com/bitcoin/bitcoin/pull/28093)
💬 hebasto commented on issue "guix: re-enable exported symbol checking for RISC-V":
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-1641848538)
Historically, symbol checking for RISC-V was skipped from the [beginning](https://github.com/bitcoin/bitcoin/pull/13724) for the following [reason](https://github.com/bitcoin/bitcoin/pull/13724#issuecomment-413652059):
> Need to skip RISC-V for now, the linker would export so many symbols.
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-1641848538)
Historically, symbol checking for RISC-V was skipped from the [beginning](https://github.com/bitcoin/bitcoin/pull/13724) for the following [reason](https://github.com/bitcoin/bitcoin/pull/13724#issuecomment-413652059):
> Need to skip RISC-V for now, the linker would export so many symbols.
💬 fanquake commented on issue "guix: re-enable exported symbol checking for RISC-V":
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-1641850050)
I don't think that's relevant anymore? I've listed the exported symbols above. It's 3.
(https://github.com/bitcoin/bitcoin/issues/28095#issuecomment-1641850050)
I don't think that's relevant anymore? I've listed the exported symbols above. It's 3.
⚠️ fanquake opened an issue: "win: non-static libssp in libbitcoinconsensus DLL"
(https://github.com/bitcoin/bitcoin/issues/28104)
If you Guix build for Windows, and call objdump on `libbitcoinconsensus-0.dll`, you'll find it uses a non-static libssp:
```bash
objdump -x src/.libs/libbitcoinconsensus-0.dll
...
The Import Tables (interpreted .idata section contents)
vma: Hint Time Forward DLL First
Table Stamp Chain Name Thunk
00263000 00263050 00000000 00000000 0026409c 00263490
DLL Name: libssp-0.dll
vma: Hint/Ord Member-Name Bound-To
2638d0 3 _
...
(https://github.com/bitcoin/bitcoin/issues/28104)
If you Guix build for Windows, and call objdump on `libbitcoinconsensus-0.dll`, you'll find it uses a non-static libssp:
```bash
objdump -x src/.libs/libbitcoinconsensus-0.dll
...
The Import Tables (interpreted .idata section contents)
vma: Hint Time Forward DLL First
Table Stamp Chain Name Thunk
00263000 00263050 00000000 00000000 0026409c 00263490
DLL Name: libssp-0.dll
vma: Hint/Ord Member-Name Bound-To
2638d0 3 _
...
💬 MarcoFalke commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#issuecomment-1641903050)
My suggestion, for future reference:
```diff
commit bfaa758ca1e690ad6d2ea34b0213f7aedc2645bb
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date: Wed Jul 19 12:41:00 2023 +0200
scripted-diff: Rename global test random context to g_mock_rand
Instead of calling it "insecure", clarify that it is mocked, so that
test failures due to the seed are hopefully more obviously attributed to
it.
-BEGIN VERIFY SCRIPT-
sed -i 's/g_insecure_rand_ctx/g_moc
...
(https://github.com/bitcoin/bitcoin/pull/27425#issuecomment-1641903050)
My suggestion, for future reference:
```diff
commit bfaa758ca1e690ad6d2ea34b0213f7aedc2645bb
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date: Wed Jul 19 12:41:00 2023 +0200
scripted-diff: Rename global test random context to g_mock_rand
Instead of calling it "insecure", clarify that it is mocked, so that
test failures due to the seed are hopefully more obviously attributed to
it.
-BEGIN VERIFY SCRIPT-
sed -i 's/g_insecure_rand_ctx/g_moc
...
🚀 fanquake merged a pull request: "rpc: doc: Added `longpollid` and `data` params to `template_request`"
(https://github.com/bitcoin/bitcoin/pull/28056)
(https://github.com/bitcoin/bitcoin/pull/28056)
✅ fanquake closed an issue: "rpc: doc: RPCHelpMan missing request parameters for getblocktemplate"
(https://github.com/bitcoin/bitcoin/issues/27998)
(https://github.com/bitcoin/bitcoin/issues/27998)
📝 MarcoFalke opened a pull request: "doc: Clarify that -fstack-reuse=all bugs exist on all versions of GCC"
(https://github.com/bitcoin/bitcoin/pull/28105)
This is a follow-up to commit 7b850bc2a1cd8547a2dbb5a18173f53439601220. While the test case no longer reproduces, the general class of `-fstack-reuse` bugs still exists in all versions of GCC. The workaround can never be removed, unless the whole class of bugs is fixed.
(https://github.com/bitcoin/bitcoin/pull/28105)
This is a follow-up to commit 7b850bc2a1cd8547a2dbb5a18173f53439601220. While the test case no longer reproduces, the general class of `-fstack-reuse` bugs still exists in all versions of GCC. The workaround can never be removed, unless the whole class of bugs is fixed.
🚀 fanquake merged a pull request: "test: remove race in the user-agent reception check"
(https://github.com/bitcoin/bitcoin/pull/27986)
(https://github.com/bitcoin/bitcoin/pull/27986)
👍 fanquake approved a pull request: "test: Add missing `set -ex` to ci/lint/06_script.sh"
(https://github.com/bitcoin/bitcoin/pull/28103#pullrequestreview-1536890199)
ACK ffff4b5dc57c32bf759b705530c1368de4aa787e
(https://github.com/bitcoin/bitcoin/pull/28103#pullrequestreview-1536890199)
ACK ffff4b5dc57c32bf759b705530c1368de4aa787e