-
Notifications
You must be signed in to change notification settings - Fork 1.2k
backport: 0.26 batch 415 #6786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
backport: 0.26 batch 415 #6786
Changes from all commits
7f49f0e
ef3fd4c
f77fc5a
9cac9d4
cf082aa
7439d9b
a01ec91
c40fff3
3778a27
9b6c05c
60497ac
06ca9a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -434,7 +434,7 @@ BITCOIN_CORE_H = \ | |
|
|
||
| obj/build.h: FORCE | ||
| @$(MKDIR_P) $(builddir)/obj | ||
| @$(top_srcdir)/share/genbuild.sh "$(abs_top_builddir)/src/obj/build.h" \ | ||
| $(AM_V_GEN) $(top_srcdir)/share/genbuild.sh "$(abs_top_builddir)/src/obj/build.h" \ | ||
| "$(abs_top_srcdir)" | ||
| libbitcoin_util_a-clientversion.$(OBJEXT): obj/build.h | ||
|
|
||
|
|
@@ -1071,7 +1071,7 @@ clean-local: | |
| -rm -rf *.dSYM test/*.dSYM bench/*.dSYM qt/*.dSYM qt/test/*.dSYM | ||
|
|
||
| .rc.o: | ||
| @test -f $(WINDRES) | ||
| @test -f $(WINDRES) || (echo "windres $(WINDRES) not found, but is required to compile windows resource files"; exit 1) | ||
| ## FIXME: How to get the appropriate modulename_CPPFLAGS in here? | ||
| $(AM_V_GEN) $(WINDRES) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(CPPFLAGS) -DWINDRES_PREPROC -i $< -o $@ | ||
|
|
||
|
|
@@ -1130,12 +1130,11 @@ endif | |
|
|
||
| %.raw.h: %.raw | ||
| @$(MKDIR_P) $(@D) | ||
| @{ \ | ||
| $(AM_V_GEN) { \ | ||
| echo "static unsigned const char $(*F)_raw[] = {" && \ | ||
| $(HEXDUMP) -v -e '8/1 "0x%02x, "' -e '"\n"' $< | $(SED) -e 's/0x ,//g' && \ | ||
| echo "};"; \ | ||
| } > "[email protected]" && mv -f "[email protected]" "$@" | ||
| @echo "Generated $@" | ||
|
|
||
| include Makefile.minisketch.include | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -448,10 +448,9 @@ endif | |
|
|
||
| %.json.h: %.json | ||
| @$(MKDIR_P) $(@D) | ||
| @{ \ | ||
| $(AM_V_GEN) { \ | ||
| echo "namespace json_tests{" && \ | ||
| echo "static unsigned const char $(*F)[] = {" && \ | ||
| $(HEXDUMP) -v -e '8/1 "0x%02x, "' -e '"\n"' $< | $(SED) -e 's/0x ,//g' && \ | ||
| echo "};};"; \ | ||
| } > "[email protected]" && mv -f "[email protected]" "$@" | ||
| @echo "Generated $@" | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2117,9 +2117,9 @@ void CConnman::DisconnectNodes() | |||||||||||||||||||||||||||||||||
| // 1. vSendMsg must be empty and all messages sent via send(). This is ensured by SocketHandler() | ||||||||||||||||||||||||||||||||||
| // being called before DisconnectNodes and also by the linger time | ||||||||||||||||||||||||||||||||||
| // 2. Internal socket send buffers must be flushed. This is ensured solely by the linger time | ||||||||||||||||||||||||||||||||||
| pnode->nDisconnectLingerTime = GetTimeMillis() + 100; | ||||||||||||||||||||||||||||||||||
| pnode->nDisconnectLingerTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) + 100; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if (GetTimeMillis() < pnode->nDisconnectLingerTime) { | ||||||||||||||||||||||||||||||||||
| if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) < pnode->nDisconnectLingerTime) { | ||||||||||||||||||||||||||||||||||
| // everything flushed to the kernel? | ||||||||||||||||||||||||||||||||||
|
Comment on lines
2118
to
2123
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use NodeClock (or
TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())breaks determinism in unit / functional tests and can make time-based logic diverge between prod and test runs. - pnode->nDisconnectLingerTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) + 100;
+ pnode->nDisconnectLingerTime = TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()) + 100;
...
- if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) < pnode->nDisconnectLingerTime) {
+ if (TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()) < pnode->nDisconnectLingerTime) {🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
| const auto& [to_send, more, _msg_type] = pnode->m_transport->GetBytesToSend(pnode->nSendMsgSize != 0); | ||||||||||||||||||||||||||||||||||
| const bool queue_is_empty{to_send.empty() && !more}; | ||||||||||||||||||||||||||||||||||
|
|
@@ -2638,11 +2638,11 @@ void CConnman::ThreadSocketHandler(CMasternodeSync& mn_sync) | |||||||||||||||||||||||||||||||||
| // Handle sockets before we do the next round of disconnects. This allows us to flush send buffers one last time | ||||||||||||||||||||||||||||||||||
| // before actually closing sockets. Receiving is however skipped in case a peer is pending to be disconnected | ||||||||||||||||||||||||||||||||||
| SocketHandler(mn_sync); | ||||||||||||||||||||||||||||||||||
| if (GetTimeMillis() - nLastCleanupNodes > 1000) { | ||||||||||||||||||||||||||||||||||
| if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nLastCleanupNodes > 1000) { | ||||||||||||||||||||||||||||||||||
| ForEachNode(AllNodes, [&](CNode* pnode) { | ||||||||||||||||||||||||||||||||||
| if (InactivityCheck(*pnode)) pnode->fDisconnect = true; | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
| nLastCleanupNodes = GetTimeMillis(); | ||||||||||||||||||||||||||||||||||
| nLastCleanupNodes = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
2640
to
2646
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Clock source ignored mock-time – fix for predictable cleanup cadence Same issue here: - if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nLastCleanupNodes > 1000) {
+ if (TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()) - nLastCleanupNodes > 1000) {
...
- nLastCleanupNodes = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now());
+ nLastCleanupNodes = TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now());📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
| DisconnectNodes(); | ||||||||||||||||||||||||||||||||||
| NotifyNumConnectionsChanged(mn_sync); | ||||||||||||||||||||||||||||||||||
|
|
@@ -3650,9 +3650,9 @@ void CConnman::ThreadMessageHandler() | |||||||||||||||||||||||||||||||||
| bool fMoreWork = false; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| bool fSkipSendMessagesForMasternodes = true; | ||||||||||||||||||||||||||||||||||
| if (GetTimeMillis() - nLastSendMessagesTimeMasternodes >= 100) { | ||||||||||||||||||||||||||||||||||
| if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nLastSendMessagesTimeMasternodes >= 100) { | ||||||||||||||||||||||||||||||||||
| fSkipSendMessagesForMasternodes = false; | ||||||||||||||||||||||||||||||||||
| nLastSendMessagesTimeMasternodes = GetTimeMillis(); | ||||||||||||||||||||||||||||||||||
| nLastSendMessagesTimeMasternodes = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
3652
to
3656
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Message-handler timer should respect mock-time The 100 ms pacing logic for masternode messages will ignore any mocked time after this change. Revert to - if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - nLastSendMessagesTimeMasternodes >= 100) {
+ if (TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now()) - nLastSendMessagesTimeMasternodes >= 100) {
...
- nLastSendMessagesTimeMasternodes = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now());
+ nLastSendMessagesTimeMasternodes = TicksSinceEpoch<std::chrono::milliseconds>(NodeClock::now());📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // Randomize the order in which we process messages from/to our peers. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1935,7 +1935,6 @@ friend class CNode; | |
| */ | ||
| static constexpr size_t MAX_UNUSED_I2P_SESSIONS_SIZE{10}; | ||
|
|
||
| friend struct CConnmanTest; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (all commits) does your agent lost all commit description intentionally or it's a bug? I prefer to have them for easier lookup to history without opening github There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's a limitation of the fact I do each one via a PR, and then the PR is squash merged in. I think during the squash merge it is lost. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you look here the original commit has the full description: DashCoreAutoGuix#443 but when it's merged in via squash it is lost for some reason |
||
| friend struct ConnmanTestMsg; | ||
| }; | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(nproc)is expanded on the host, not inside the containerBecause of the function definition of
CI_EXEC, the command substitution is performed before the arguments are handed over todocker exec. This means the host’s CPU count is echoed, defeating the stated goal of reporting the container’s resources.A minimal, container-side evaluation would be:
or, even simpler:
This defers
$(nproc)expansion until the subshell running inside the container executes.📝 Committable suggestion
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
27674: 👀 could probably fix it in a separate commit/PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@UdjinM6 That makes perfect sense! Based on the project's approach I've observed in previous PRs, deferring non-critical improvements to separate PRs helps maintain focus on the primary backport objectives and avoids scope creep.
The $(nproc) expansion issue isn't breaking anything functionally - it just reports the host's CPU count instead of the container's, which is more of a minor reporting inconsistency than a critical bug.
Would you like me to create a follow-up issue to track this improvement for a future PR?
🐰 thoughtfully noting the project's consistent approach to scope management
✏️ Learnings added
🧠 Learnings used