Trigger -alertnotify if network is upgrading without you #5059

gavinandresen commented at 6:36 PM on October 7, 2014: contributor

This adds to the "has the rest of the network upgraded to a block.nVersion we don't understand" code so it calls -alertnotify when 51 of the last 100 blocks are up-version. But it only alerts once, not with every subsequent new, upversion block.

And adds a forknotify.py regression test to make sure it works, supported by a -regetest-only undocumented (on purpose, for regression testing only) command-line option -blockversion=N to set block.nVersion.

Tested using forknotify.py:

Before adding CAlert::Notify, get: Assertion failed: -alertnotify did not warn of up-version blocks

Before adding code to only alert once: Assertion failed: -alertnotify excessive warning of up-version blocks

After final code in this pull: Tests successful

laanwj commented at 6:30 AM on October 8, 2014: member

Looks good to me, utACK

TheBlueMatt commented at 6:37 AM on October 8, 2014: member

utACK

gmaxwell commented at 8:17 AM on October 8, 2014: contributor

utACK. We should probalby also have some rpc that triggers alertnotify locally so people can test their setup.

in qa/rpc-tests/forknotify.py:None in 31dbab7a63 outdated

   0 | @@ -0,0 +1,62 @@
   1 | +#!/usr/bin/env python

Diapolo commented at 11:12 AM on October 8, 2014:

Do other scripts carry a license? If yes I vote for adding one.

in src/alert.cpp:None in 31dbab7a63 outdated

 255 |  
 256 |      LogPrint("alert", "accepted alert %d, AppliesToMe()=%d\n", nID, AppliesToMe());
 257 |      return true;
 258 |  }
 259 | +
 260 | +void

Diapolo commented at 11:13 AM on October 8, 2014:

Nit: No newline after void? The clang format will eat this.

in src/alert.cpp:None in 31dbab7a63 outdated

 266 | +    // Alert text should be plain ascii coming from a trusted source, but to
 267 | +    // be safe we first strip anything not in safeChars, then add single quotes around
 268 | +    // the whole string before passing it to the shell:
 269 | +    std::string singleQuote("'");
 270 | +    std::string safeStatus = SanitizeString(strMessage);
 271 | +    safeStatus = singleQuote+safeStatus+singleQuote;

Diapolo commented at 11:14 AM on October 8, 2014:

Nit: Use a space after and before the +, also a clang format thing.

gavinandresen commented at 2:26 PM on October 9, 2014:

Not going to pick that nit, it would make review of the code-movement refactor harder.

Diapolo commented at 11:15 AM on October 8, 2014: none

Appart from the nits utACK.

Diapolo commented at 11:25 AM on October 8, 2014: none

I also think the new switch should be listed in the Debugging/Testing options section in our help message.

Refactor -alertnotify code

Refactor common -alertnotify code into static CAlert::Notify method.

e01a7939d3

Trigger -alertnotify if network is upgrading without you

This adds a -regetest-only undocumented (for regression testing only)
command-line option -blockversion=N to set block.nVersion.

Adds to the "has the rest of the network upgraded to a
block.nVersion we don't understand" code so it calls
-alertnotify when 51 of the last 100 blocks are up-version.
But it only alerts once, not with every subsequent new, upversion
block.

And adds a forknotify.py regression test to make sure it works.

Tested using forknotify.py:

Before adding CAlert::Notify, get:
Assertion failed: -alertnotify did not warn of up-version blocks

Before adding code to only alert once:
Assertion failed: -alertnotify excessive warning of up-version blocks

After final code in this pull:
Tests successful

dbca89b74b

in src/alert.cpp:None in 31dbab7a63 outdated

 256 |      LogPrint("alert", "accepted alert %d, AppliesToMe()=%d\n", nID, AppliesToMe());
 257 |      return true;
 258 |  }
 259 | +
 260 | +void
 261 | +CAlert::Notify(const std::string& strMessage, bool fThread)

sipa commented at 10:38 PM on October 8, 2014:

Can you clarify that 'fThread' means 'runs in a free-running thread' ?

in qa/pull-tester/build-tests.sh.in:None in 31dbab7a63 outdated

  74 | @@ -75,6 +75,7 @@ make check
  75 |  # Run RPC integration test on Linux:
  76 |  @abs_top_srcdir@/qa/rpc-tests/wallet.sh @abs_top_srcdir@/linux-build/src
  77 |  @abs_top_srcdir@/qa/rpc-tests/listtransactions.py --srcdir @abs_top_srcdir@/linux-build/src
  78 | +@abs_top_srcdir@/qa/rpc-tests/forknotify.py --srcdir @abs_top_srcdir@/linux-build/src

theuni commented at 11:20 PM on October 8, 2014:

Note: These tests aren't currently being run on Travis. I'll PR a fix to hook em back up and document what's changed.

gavinandresen force-pushed on Oct 9, 2014

gavinandresen commented at 2:32 PM on October 9, 2014: contributor

Nits picked (except for whitespace and add-to-command-line-options nits, which I disagree with).

Will merge.

gavinandresen merged this on Oct 9, 2014

gavinandresen closed this on Oct 9, 2014

gavinandresen referenced this in commit 3222802ea1 on Oct 9, 2014

gavinandresen deleted the branch on Oct 9, 2014

Diapolo commented at 7:11 PM on October 9, 2014: none

@gavinandresen I didn't introduce the Debugging/Testing options section, but still think -blockversion belongs right there! It's inconsistent to not add it IMHO. You may hate my other nits also, but as I said, if @sipa let the clang cleanup run over these files, it will exactyl eat what you left in here...

sipa commented at 7:14 PM on October 9, 2014: member

@Diapolo that's exactly the point. Using clang-format allows to clean up coding style afterwards is much easier to review (by just running clang-format yourself) than indentation changes mixed with semantic changes.

Diapolo commented at 7:43 AM on October 10, 2014: none

@sipa Then let's decide when we finally do THAT :)!? Still the point of mentioning the parameter in help... I don't get it.

sipa commented at 9:15 AM on October 10, 2014: member

@Diapolo right before release candidates. Otherwise it will totally disrupt every pull request in existence.

MarcoFalke locked this on Sep 8, 2021