test: add functional test for -startupnotify #23532

This PR adds a functional test for -startupnotify. It basically starts the node passing a command on -startupnotify to create a file on tmp and then, we check if the file has been successfully created.

Concept ACK.

Perhaps the test with valid command can create a file in test or tmp dir and checks if this file was created..

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22776 (rpc/wallet: add optional transaction(s) to getbalances by kallewoof)
• #22751 (rpc/wallet: add simulaterawtransaction RPC by kallewoof)

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.

I've tried different valid commands with -startupnotify like: echo (common command for Unix system and Windows), tried a simple ls for Unix and dir for Windows and other ones, but the CI is failing for all of them. Any thoughts about it, @MarcoFalke?

You can find the error in the log:

 node0 stderr Error: The experimental syscall sandbox feature (-sandbox=<mode>) is incompatible with -startupnotify (which uses execve). 

You can find the error in the log:

 node0 stderr Error: The experimental syscall sandbox feature (-sandbox=<mode>) is incompatible with -startupnotify (which uses execve). 

Sorry, I read that but didn't notice this part of the log. Thanks.

Concept ACK

Force-pushed with a update on expected_stderr in stop_node. I just edited the description.

Win64 has failed: <img width="1060" alt="Screen Shot 2021-11-18 at 10 23 47" src="https://user-images.githubusercontent.com/19480819/142430123-5ed41a77-3173-4442-85ae-6f37654921b6.png">

I think the problem may be the breaklines. Going to work to fix it.

Force-pushed adding os.linesep into expected windows message.

Ideally, a test for -startupnotify should ensure that the command is executed exactly once, and when it is executed, the node is fully started (which may involve races which make it difficult to deterministically fail).

Force-pushed addressing the reviews, thanks everyone for them. I just changed the approach for this test (updated the description).

ACK 31931a5ab009256bf583025939e6e3ca2f875d9a

A couple of ideas, feel free to ignore/pick/choose:

• Begin by testing the absence of tmpdir without -startupnotify before asserting on its presence with the option
• Maybe abstract the magic strings to constants

 from test_framework.test_framework import BitcoinTestFramework
 
+NODE_DIR = "node0"
+FILE_NAME = "test_123_xyz"
 
 class StartupNotifyTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
         self.disable_syscall_sandbox = True
-        self.extra_args = [["-startupnotify=echo 'test' >> node0/test.txt"]]
 
     def run_test(self):
-        tmpdir = os.path.join(self.options.tmpdir, "node0", "test.txt")
+        tmpdir = os.path.join(self.options.tmpdir, NODE_DIR, FILE_NAME)
+        assert not os.path.exists(tmpdir)
 
         self.log.info("When node starts, check if test.txt exists")
+        self.restart_node(0, extra_args=[f"-startupnotify=echo '{FILE_NAME}' >> {NODE_DIR}/{FILE_NAME}"])
         assert os.path.exists(tmpdir)

Ideally, a test for -startupnotify should ensure that the command is executed exactly once, and when it is executed, the node is fully started (which may involve races which make it difficult to deterministically fail).

Maybe checking the expected result of an RPC (like getblockcount) could verify the node is fully up.

Force-pushed addressing @luke-jr and @jonatack tips. Thanks for the reviews.

ACK, modulo unrelated code that appears to have crept into the last push?

ACK, modulo unrelated code that appears to have crept into the last push?

omg, didn't understand why it happened, going to check it.

fixed commits! @jonatack

tested ACK 98014c5

OS: Arch Linux 64 bits (Linux 5.12.19-hardened1-1-hardened x86_64) System: AMD Ryzen 5 64 bits + AMD VEGA 8 with 12GB RAM Build: Compiled from source with no additional ./configure options

Tests: Ran the test alone, it worked! Also, all the tests with test/functional/test_runner.py, again everything is working just fine!

force-pushed addressing @luke-jr and @kristapsk reviews (check if the command is executed once).

ACK dbdcc783e39794d54c2f17040d248539b1ca46b0

reACK dbdcc78

Nice addition to test coverage.

feature_startupnotify.py                           | ✖ Failed  | 2 s

feature_startupnotify.py                           | ✖ Failed  | 2 s

Fixed!

Tested ACK 126853214a490ee840e83ca17c717c40cfbe6837 (run on OpenBSD 7.0)

re-ACK 126853214a490ee840e83ca17c717c40cfbe6837

Concept and approach ACK. Tested and working fine :) I saw that startupnotify uses execve and I was wondering if that could be checked on the test, somehow like: in case of using sandbox (if defined(USE_SYSCALL_SANDBOX), startupnotify will not throw?