test: Extend stale tip test

maflcko commented at 11:45 am on March 30, 2020: member

Extend test with ASSERT_DEBUG_LOG
Extend test with case where a block is in flight from a node that is about to be evicted

maflcko added the label P2P on Mar 30, 2020

DrahtBot commented at 12:27 pm on March 30, 2020: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	jonatack, fanquake, practicalswift
Stale ACK	fridokus

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27385 (net, refactor: extract Network and BIP155Network logic to node/network by jonatack)
#27213 (p2p: Diversify automatic outbound connections with respect to networks by amitiuttarwar)
#25908 (p2p: remove adjusted time by fanquake)
#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#25268 (refactor: Introduce EvictionManager by dergoegge)
#23352 (test: Extend stale_tip_peer_management test by MarcoFalke)
#17860 (fuzz: BIP 42, BIP 30, CVE-2018-17144 by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Coverage

Coverage	Change (pull 18470, 333634883d442ef0a9bd57e399f4cc97003e624f)	Reference (master, 5f9cd62f33fb4d440173b9c376cadf4887e81e9d)
Lines	+0.0553 %	90.2454 %
Functions	+0.2530 %	85.9695 %
Branches	-0.0140 %	51.7670 %

Updated at: 2020-03-30T13:59:49.217418.

fridokus commented at 1:32 pm on March 30, 2020: contributor

ACK faaba842d03762591f8612fd1fb16c93c881913f Ran functional tests on Ubuntu 16 and glanced through the code.

maflcko force-pushed on Mar 30, 2020

fridokus commented at 12:22 pm on March 31, 2020: contributor

ACK fadb1acd1392d42b08d5b27360764c83e94b42ae Ran functional tests.

DrahtBot added the label Needs rebase on Apr 8, 2020

maflcko force-pushed on Apr 8, 2020

DrahtBot removed the label Needs rebase on Apr 8, 2020

DrahtBot added the label Needs rebase on Apr 15, 2020

jonatack commented at 9:27 pm on May 5, 2020: contributor

Concept ACK to be notified on rebase.

in src/net_processing.cpp:1153 in fa579ee742 outdated

757@@ -758,6 +758,7 @@ void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds)
758     LOCK(cs_main);
759     CNodeState *state = State(node);
760     if (state) state->m_last_block_announcement = time_in_seconds;
761+    if (state) state->fHaveWitness = true;

fanquake commented at 10:49 am on May 27, 2020:

This seems unrelated to std::chrono:: / time refactoring.

in src/net_processing.cpp:3507 in fa579ee742 outdated

3506+    if (time > m_stale_tip_check_time) {
3507         // Check whether our tip is stale, and if so, allow using an extra
3508         // outbound peer
3509         if (!fImporting && !fReindex && connman->GetNetworkActive() && connman->GetUseAddrmanOutgoing() && TipMayBeStale(consensusParams)) {
3510-            LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", time_in_seconds - g_last_tip_update);
3511+            LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", count_seconds(time - g_last_tip_update.load()));

fanquake commented at 10:59 am on May 27, 2020:

Can you clarify why you’re using count_seconds here rather than .load().count()?

maflcko commented at 11:22 am on May 27, 2020:

The type of time - g_last_tip_update.load() should be assumed to be std::chrono::something, but the exact type should be assumed to be unknown, since it can change in a future refactor. Therefore, we need to somehow specify what precision we want to log to the debug log.

Using the count_* helpers is one way and I bet there are also other ways to achieve the same.

maflcko commented at 1:35 pm on May 27, 2020:

I recommend reviewing #18638 , which adds documentation to count_seconds

fanquake commented at 11:15 am on May 27, 2020: member

Concept ACK

faa01d605c4a4aef50c48ba3e85bbea517235abb and faee067333657a22a38955db88a9401e67184336 look good.

As for fa579ee742e608ec2957ce3762ed05eb76997c3f; +1 for moving to std::chrono. However there seems to be some unrelated changes bundled into this commit. Didn’t look at the test changes too hard.

fanquake commented at 12:52 pm on June 9, 2021: member

@MarcoFalke could you rebase this (if there isn’t any other PR that should be reviewed first), and comment in regards to: #18470 (review).

maflcko force-pushed on Jun 9, 2021

maflcko added the label Waiting for author on Jun 9, 2021

DrahtBot removed the label Needs rebase on Jun 9, 2021

fanquake referenced this in commit 551933f9ec on Jun 11, 2021

maflcko force-pushed on Jun 11, 2021

sidhujag referenced this in commit 5d4e00cdc3 on Jun 11, 2021

DrahtBot added the label Needs rebase on Jul 28, 2021

practicalswift commented at 7:21 pm on July 29, 2021: contributor

Concept ACK: more type safety is better :)

maflcko force-pushed on Oct 25, 2021

DrahtBot removed the label Needs rebase on Oct 25, 2021

DrahtBot added the label Needs rebase on Nov 18, 2021

maflcko removed the label Waiting for author on Aug 1, 2022

fanquake commented at 10:06 am on August 22, 2022: member

Looks like a number of the changes here have been merged via other PRs. i.e 1ee07b5e207771f68b1686804c3eb474f7f6648f has been done by fa9086d085f664a96561eeb5dd29fc1a4e4f926a. Most-of, if not all the net_processing refactoring has also now been done.

maflcko renamed this:
~~net: Make stale tip check time type-safe, extend test~~
test: Extend stale tip test
on Aug 22, 2022

maflcko removed the label P2P on Aug 22, 2022

maflcko force-pushed on Aug 22, 2022

maflcko marked this as a draft on Aug 22, 2022

DrahtBot removed the label Needs rebase on Aug 22, 2022

DrahtBot added the label Tests on Aug 22, 2022

DrahtBot added the label Needs rebase on Aug 30, 2022

achow101 commented at 6:06 pm on October 12, 2022: member

Are you still working on this?

net: Extend stale tip test

* Extend test with ASSERT_DEBUG_LOG
* Extend test with case where a block is in flight from a node that is
  about to be evicted

faac87d5ef

maflcko force-pushed on Jan 11, 2023

DrahtBot removed the label Needs rebase on Jan 11, 2023

DrahtBot commented at 11:56 am on May 6, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on May 6, 2023

DrahtBot commented at 1:42 am on August 4, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

furszy commented at 2:02 pm on August 4, 2023: member

This test extension could play well with #28170. Are you planning to rebase it? Happy to review it.

achow101 added the label Up for grabs on Sep 23, 2023

achow101 commented at 4:44 pm on September 23, 2023: member

Closing as up for grabs due to lack of activity.

achow101 closed this on Sep 23, 2023

maflcko removed the label Up for grabs on Feb 6, 2024

maflcko deleted the branch on Feb 6, 2024

test: Extend stale tip test #18470

Reviews

Conflicts

Coverage