test: Add unit & functional test coverage for blockstore by pinheadmz · Pull Request #27850 · bitcoin/bitcoin

pinheadmz · 2023-06-10T13:14:19Z

This PR adds unit and functional tests to cover the behavior described in #2039. In particular, that bitcoind will crash on startup if a reindex is requested but the blk files are read-only. Eventually this behavior can be updated with #27039. This PR just commits the test coverage from #27039 as suggested in #27039 (comment)

DrahtBot · 2023-06-10T13:14:21Z

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
ACK	jonatack, MarcoFalke, achow101
Concept ACK	dergoegge
Stale ACK	ryanofsky, furszy

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:

#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

dergoegge · 2023-06-12T10:07:27Z

Concept ACK

pinheadmz · 2023-06-13T19:27:26Z

I fixed the CI failures by skipping tasks that are run as root. Root privileges override the read-only file permissions and the test fails because we expect an error that doesn't throw (can not open read-only file in write mode).

There are 6 tasks that skip this test for this reason:

[ASan + LSan + UBSan + integer, no depends, USDT] [lunar]
[multiprocess, i686, DEBUG] [focal]
[previous releases, qt5 dev package and depends packages, DEBUG] [focal]
[TSan, depends, gui] [lunar]
32-bit + dash [gui] [CentOS 9]
[no wallet, libbitcoinkernel] [focal]

pinheadmz · 2023-06-14T11:17:44Z

Ready for review now, CI is passing modulo #27879

willcl-ark · 2023-06-14T12:07:15Z

Just curious, did you try using chattr +i to set blockfile as immutable ? May break other stuff, I don't know...

Seems a shame to miss linux out.

ref: https://man7.org/linux/man-pages/man1/chattr.1.html

pinheadmz · 2023-06-14T14:13:35Z

Just curious, did you try using chattr +i to set blockfile as immutable ? May break other stuff, I don't know...

Bold strategy, Cotton let's see if it pays off: cd394b637f

maflcko · 2023-08-23T08:55:32Z

You can find the error in the logs:

2023-08-22T19:24:19.533000Z TestFramework (WARNING): Can not make file immutable (Command '['chattr', '+i', WindowsPath('C:/Users/ContainerAdministrator/AppData/Local/Temp/test_runner_₿_🏃_20230822_185732/feature_reindex_readonly_206/node0/regtest/blocks/blk00000.dat')]' returned non-zero exit status 1.), trying read-only instead
2023-08-22T19:24:20.156000Z TestFramework (INFO): Stopping nodes
2023-08-22T19:24:20.156000Z TestFramework (WARNING): Not cleaning up dir C:\Users\ContainerAdministrator\AppData\Local\Temp\test_runner_₿_🏃_20230822_185732\feature_reindex_readonly_206
2023-08-22T19:24:20.156000Z TestFramework (INFO): Tests successful
stderr:
Usage: chattr [-RVfHv] [+-=mode]... [file]...
Try 'chattr --help' for more information

pinheadmz · 2023-08-23T10:24:57Z

You can find the error in the logs:

Right but that error is caught in a try and printed as a warning. On macos, chattr doesn't exist:

2023-08-22T20:01:29.247000Z TestFramework (WARNING): Can not make file immutable ([Errno 2] No such file or directory: 'chattr'), trying read-only instead

I guess the difference is on windows chattr is installed but syntax is different so it throws a different error that gets logged to stderr... is that why test framework is failing the test? Even though it is "Tests successful"

maflcko · 2023-08-23T10:32:26Z

Ah right. It is not allowed to print anything to stderr during the test. You'll have to catch that as well.

See

                    if proc.returncode == TEST_EXIT_PASSED and stderr == "":
                        status = "Passed"
                    elif proc.returncode == TEST_EXIT_SKIPPED:
                        status = "Skipped"
                        skip_reason = re.search(r"Test Skipped: (.*)", stdout).group(1)
                    else:
                        status = "Failed"

maflcko · 2023-08-24T07:42:05Z

                                   FileNotFoundError: [Errno 2] No such file or directory: 'chattr'

Maybe need to install it?

pinheadmz · 2023-08-25T17:55:48Z

Maybe need to install it?

Added e2fsprogs in centos containers with force-push

maflcko · 2023-08-26T06:41:27Z

nit: May be better to put this into CI_BASE_PACKAGES, both for centos, and ubuntu/debian to avoid failures when it is removed from the vanilla ubuntu/debian container image.

good idea, done

maflcko

lgtm, thanks for taking the feedback so far

pinheadmz · 2023-08-26T11:15:25Z

lgtm, thanks for taking the feedback so far

thanks for your stamina on this one as well! >100 comments 👍

maflcko · 2023-08-27T17:10:16Z

Only changes since last review:

Rebase
Replace non-root user with chattr
Drop unused test node

maflcko

left two nits/questions.

re-ACK 6cda1acdea2b908345406389baaef593ac5cce76 🤼

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 6cda1acdea2b908345406389baaef593ac5cce76 🤼
dakRHCgn0dA/yjlDWocN/nVHFZ2qhu24JHr3Kf+ETosirJP1raLsjYzRUHm5kFp04VPWj0OXABmzLNxYXMymCA==

maflcko · 2023-08-27T17:02:08Z

style nit in 6cda1acdea: Can remove this?

thanks fixed

jonatack · 2023-08-27T17:58:35Z

Suggested change

self.log.warning(f"Can not make file immutable with chattr, trying chmod instead")

self.log.info("Cannot make file immutable with chattr, trying chmod instead")

Would suggest logging this as info or debug rather than warning, as it is platform-specific and not an actionable warning for the user.

Drop unneeded f-string.

s/Can not/Cannot/ or s/Can not/Unable to/

Not a blocker, but I need the same logic for #28116, if you feel like making a test framework utility method as you did previously here.

Thanks Jon I'll change the log level but leave the utility method for a follow up to keep revisions minimal

I need the same logic for #28116, if you feel like making a test framework utility method

Now extracted to a test framework utility in #28116.

pinheadmz · 2023-09-05T13:10:53Z

force-pushed because I accidentally squashed two commits together. Diff is null 278e675 to 9791b31

jonatack

ACK 9791b3182514a2571a7c1cdb38b59338c24d974d

jonatack · 2023-09-05T16:42:56Z

259889d Maybe assert the value here before the read, to ensure that it changed with the BOOST_CHECK_EQUAL(read_block.nVersion, 1); check that follows.

CBlock read_block; + BOOST_CHECK_EQUAL(read_block.nVersion, 0); {

jonatack · 2023-09-05T16:44:29Z

9791b31

suggest making the other added logs info, or this log debug

s/Can not/Cannot/ and mention that chattr is platform-specific to linux (see suggestion)

missing line breaks

@@ -11,6 +11,7 @@ import stat from test_framework.test_framework import BitcoinTestFramework + class BlockstoreReindexTest(BitcoinTestFramework): @@ -41,7 +42,7 @@ class BlockstoreReindexTest(BitcoinTestFramework): if not used_chattr: - self.log.info("Can not make file immutable with chattr, trying chmod instead") + self.log.debug("Cannot make file read-only with chattr (which is linux-only), using chmod instead") @@ -57,5 +58,6 @@ class BlockstoreReindexTest(BitcoinTestFramework): self.reindex_readonly() + if __name__ == '__main__': BlockstoreReindexTest().main()

$ pycodestyle test/functional/feature_reindex_readonly.py test/functional/feature_reindex_readonly.py:14:1: E302 expected 2 blank lines, found 1 test/functional/feature_reindex_readonly.py:60:1: E305 expected 2 blank lines after class or function definition, found 1

I generally prefer chatty tests, which makes it easier to see progress and see what the test is currently doing, as long as the output is not high-frequent. So I think the log level can be left as-is.

maflcko

re-ACK 9791b3182514a2571a7c1cdb38b59338c24d974d 🕕

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 9791b3182514a2571a7c1cdb38b59338c24d974d 🕕
Yj2uRwzpT1GD2q/ibV1KkO+mrHtJd2SSWN8t4DWILeAQrpp52zK2SWHQFhk/iD04RqlyGEtI32q3lRbj3NNvBg==

maflcko · 2023-09-06T07:46:21Z

why two

newlines?

nit: I think this wasn't addressed?

maflcko · 2023-09-06T07:51:25Z

Any reason to add an alias, but not use it consistently?

maflcko · 2023-09-06T07:52:13Z

Why is this needed, when the same call was already done unconditionally, which seems preferable than a conditional call?

maflcko · 2023-09-06T07:53:07Z

Any reason for the else? Seems better to also do this unconditionally, because setting it was done unconditionally.

Otherwise it may cause edge cause test failures on some machines/configs only.

maflcko · 2023-09-06T07:54:33Z

Suggested change

# Copyright (c) 2023 The Bitcoin Core developers

# Copyright (c) 2023-present The Bitcoin Core developers

nit: For new code could use -present (or omit the year) to avoid having to ever touch it again.

pinheadmz · 2023-09-06T18:04:28Z

Thanks again @MarcoFalke and @jonatack I addressed all your feedback

jonatack

ACK f1264f9baca4207c4c530f01d90fcdc841368126

maflcko

re-ACK f1264f9baca4207c4c530f01d90fcdc841368126 🍦

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK f1264f9baca4207c4c530f01d90fcdc841368126 🍦
G5HM6LhMQyxLvXfOSzHbPsljWaZ9XpmswhOHqpwakw2n06WWG+7NyED+63/6l0Lcy6tPvu6axWBFhHvMZKAMAA==

achow101 · 2023-09-14T15:06:03Z

ACK f1264f9baca4207c4c530f01d90fcdc841368126

achow101 · 2023-09-14T15:29:55Z

When this test is run as non-root, this results in something being written to stderr which will cause the test runner to always fail. That's really annoying for testing locally.

Suggested change

try:

subprocess.check_call(['chattr', '+i', filename])

used_chattr = True

self.log.info("Made file immutable with chattr")

except Exception:

pass

try:

subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)

used_chattr = True

self.log.info("Made file immutable with chattr")

except subprocess.CalledProcessError as e:

self.log.warning(str(e))

if e.stdout:

self.log.warning(f"stdout: {e.stdout}")

if e.stderr:

self.log.warning(f"stderr: {e.stderr}")

Added! thanks for the patch

Co-authored-by: Andrew Chow <github@achow101.com>

jonatack · 2023-09-14T16:19:22Z

ACK de8f912 tested on macOS, but not on Linux for the Linux-related change in the last push

maflcko

lgtm ACK de8f912 📶

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK de8f9123afbecc3b4f59fa80af8148bc865d0588 📶
aCT7KL7/lymQNqrfkzsZyKFtz7sH6ZgYVwranWjN6pDe/m5Cd47wdd2X8+yokXAAQkqj7TMxrlrauXR514LfBw==

achow101 · 2023-09-14T17:15:30Z

ACK de8f912

maflcko · 2023-10-07T11:05:01Z

+        filename.chmod(stat.S_IREAD)
+
+        used_chattr = False
+        if platform.system() == "Linux":


I guess this test will also fail on freebsd when running as root

A solution may be to check for freebsd and chflags?

I have an OpenBSD server I can run the test on, will follow up with a PR this week

pinheadmz mentioned this pull request Jun 10, 2023

blockstorage: do not flush block to disk if it is already there #27039

Merged

pinheadmz force-pushed the blockstore-tests branch from b58d3fa to 4daeaa5 Compare June 10, 2023 13:27

DrahtBot added the CI failed label Jun 10, 2023

pinheadmz mentioned this pull request Jun 10, 2023

Return EXIT_FAILURE on post-init fatal errors #27708

Merged

pinheadmz force-pushed the blockstore-tests branch 2 times, most recently from ba8547f to 38bc463 Compare June 10, 2023 23:43

furszy reviewed Jun 11, 2023

View reviewed changes

Comment thread src/test/blockmanager_tests.cpp Outdated

Comment thread test/functional/feature_reindex.py Outdated

pinheadmz marked this pull request as draft June 11, 2023 16:52

DrahtBot changed the title ~~Add unit & functional test coverage for blockstore~~ test: Add unit & functional test coverage for blockstore Jun 12, 2023

pinheadmz force-pushed the blockstore-tests branch 4 times, most recently from 6f7499a to e01ac84 Compare June 12, 2023 20:22

DrahtBot mentioned this pull request Jun 13, 2023

blockstorage: Return on fatal flush errors #27866

Merged

maflcko reviewed Jun 13, 2023

View reviewed changes

Comment thread test/functional/feature_reindex.py Outdated

pinheadmz force-pushed the blockstore-tests branch from e01ac84 to f88b87b Compare June 13, 2023 14:28

DrahtBot mentioned this pull request Jun 13, 2023

kernel: Rm ShutdownRequested and AbortNode from validation code. #27861

Merged

pinheadmz force-pushed the blockstore-tests branch 2 times, most recently from f37e904 to da1f187 Compare June 13, 2023 19:22

pinheadmz force-pushed the blockstore-tests branch 2 times, most recently from 689448d to ae4d5f5 Compare June 14, 2023 01:07

pinheadmz marked this pull request as ready for review June 14, 2023 11:15

pinheadmz force-pushed the blockstore-tests branch from ae4d5f5 to 5bc5150 Compare June 14, 2023 13:50

pinheadmz force-pushed the blockstore-tests branch from cd394b6 to 1dd0dfd Compare June 14, 2023 14:47

pinheadmz force-pushed the blockstore-tests branch from bee4e3f to 0ff660f Compare August 23, 2023 15:02

maflcko reviewed Aug 26, 2023

View reviewed changes

maflcko approved these changes Aug 26, 2023

View reviewed changes

maflcko approved these changes Aug 27, 2023

View reviewed changes

jonatack reviewed Aug 27, 2023

View reviewed changes

DrahtBot mentioned this pull request Aug 28, 2023

ci: Remove /ro_base bind mount, Remove LC_ALL=C from s390x task #28353

Merged

jonatack reviewed Sep 5, 2023

View reviewed changes

maflcko approved these changes Sep 6, 2023

View reviewed changes

pinheadmz added 2 commits September 6, 2023 10:39

unit test: add coverage for BlockManager

e573f24

ci: enable chattr +i capability inside containers

5c2185b

jonatack reviewed Sep 7, 2023

View reviewed changes

maflcko approved these changes Sep 14, 2023

View reviewed changes

achow101 reviewed Sep 14, 2023

View reviewed changes

test: cover read-only blockstore

de8f912

Co-authored-by: Andrew Chow <github@achow101.com>

maflcko approved these changes Sep 14, 2023

View reviewed changes

Comment thread test/functional/feature_reindex_readonly.py

ryanofsky mentioned this pull request Sep 20, 2023

Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

Merged

maflcko reviewed Oct 7, 2023

View reviewed changes

pinheadmz mentioned this pull request Oct 16, 2023

test: enable reindex readonly test on *BSD #28660

Merged

	self.log.warning(f"Can not make file immutable with chattr, trying chmod instead")
	self.log.info("Cannot make file immutable with chattr, trying chmod instead")

	# Copyright (c) 2023 The Bitcoin Core developers
	# Copyright (c) 2023-present The Bitcoin Core developers

-            try:
-                subprocess.check_call(['chattr', '+i', filename])
-                used_chattr = True
-                self.log.info("Made file immutable with chattr")
-            except Exception:
-                pass
+            try:
+                subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
+                used_chattr = True
+                self.log.info("Made file immutable with chattr")
+            except subprocess.CalledProcessError as e:
+                self.log.warning(str(e))
+                if e.stdout:
+                    self.log.warning(f"stdout: {e.stdout}")
+                if e.stderr:
+                    self.log.warning(f"stderr: {e.stderr}")

Uh oh!

Conversation

pinheadmz commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

dergoegge commented Jun 12, 2023

Uh oh!

Uh oh!

pinheadmz commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinheadmz commented Jun 14, 2023

Uh oh!

willcl-ark commented Jun 14, 2023

Uh oh!

pinheadmz commented Jun 14, 2023

Uh oh!

maflcko commented Aug 23, 2023

Uh oh!

pinheadmz commented Aug 23, 2023

Uh oh!

maflcko commented Aug 23, 2023

Uh oh!

maflcko commented Aug 24, 2023

Uh oh!

pinheadmz commented Aug 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Aug 26, 2023

Uh oh!

maflcko commented Aug 27, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Sep 5, 2023

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pinheadmz commented Jun 10, 2023 •

edited

Loading

DrahtBot commented Jun 10, 2023 •

edited

Loading

pinheadmz commented Jun 13, 2023 •

edited

Loading

jonatack Aug 27, 2023 •

edited

Loading

jonatack Sep 5, 2023 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack commented Sep 14, 2023 •

edited

Loading