test: Add unit & functional test coverage for blockstore #27850

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.

src/test/blockmanager_tests.cpp

test/functional/feature_reindex.py

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

Concept ACK

test/functional/feature_reindex.py

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: cd394b6

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

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

jonatack

ACK 9791b31

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

src/test/blockmanager_tests.cpp

+    // First two blocks are written as expected
+    // Errors are expected because block data is junk, thrown AFTER successful read
+    CBlock read_block;
+    {


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

test/functional/feature_reindex_readonly.py

+                pass
+
+        if not used_chattr:
+            self.log.info("Can not make file immutable with chattr, trying chmod instead")


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 9791b31 🕕

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

ci/test/04_install.sh

@@ -31,7 +31,9 @@ if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then
  fi

  # shellcheck disable=SC2086
-  CI_CONTAINER_ID=$(docker run $CI_CONTAINER_CAP --rm --interactive --detach --tty \
+
+


why two

newlines?

nit: I think this wasn't addressed?

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

test/functional/feature_reindex_readonly.py

+        self.log.debug("Make the first block file read-only")
+        (self.nodes[0].chain_path / "blocks" / "blk00000.dat").chmod(stat.S_IREAD)
+
+        filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"


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

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

test/functional/feature_reindex_readonly.py

+
+        if not used_chattr:
+            self.log.info("Can not make file immutable with chattr, trying chmod instead")
+            (self.nodes[0].chain_path / "blocks" / "blk00000.dat").chmod(stat.S_IREAD)


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

test/functional/feature_reindex_readonly.py

+
+        if used_chattr:
+            subprocess.check_call(['chattr', '-i', filename])
+        else:


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

test/functional/feature_reindex_readonly.py

@@ -0,0 +1,61 @@
+#!/usr/bin/env python3
+# Copyright (c) 2023 The Bitcoin Core developers


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 f1264f9

maflcko

re-ACK f1264f9 🍦

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 f1264f9

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

test/functional/feature_reindex_readonly.py

+            try:
+                subprocess.check_call(['chattr', '+i', filename])
+                used_chattr = True
+                self.log.info("Made file immutable with chattr")
+            except Exception:
+                pass


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==

test/functional/feature_reindex_readonly.py

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

ACK de8f912

…ockstore de8f912 test: cover read-only blockstore (Matthew Zipkin) 5c2185b ci: enable chattr +i capability inside containers (Matthew Zipkin) e573f24 unit test: add coverage for BlockManager (Matthew Zipkin) Pull request description: This PR adds unit and functional tests to cover the behavior described in bitcoin#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 bitcoin#27039. This PR just commits the test coverage from bitcoin#27039 as suggested in bitcoin#27039 (comment) ACKs for top commit: jonatack: ACK de8f912 modulo suggestions in bitcoin#27850 (comment), tested on macOS, but not on Linux for the Linux-related change in the last push achow101: ACK de8f912 MarcoFalke: lgtm ACK de8f912 📶 Tree-SHA512: b9bd684035dcea11c901b649fc39f397a2155a9a8459f3348e67947e387e45312fddeccb52981aef486f8a31deebb5356a7901c1bb94b78f82c24192a369af73

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

test/functional/feature_reindex_readonly.py

+        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

5a0688a test: enable reindex readonly test on *BSD and macOS as root (Matthew Zipkin) Pull request description: see bitcoin/bitcoin#27850 (comment) OpenBSD and FreeBSD don't have `chattr` but they do have `chflags`, use that method to make the block file immutable for the reindex_readonly test. Written and tested on a VPS running FreeBSD: ``` FreeBSD freebsd-13-1 13.2-RELEASE-p4 FreeBSD 13.2-RELEASE-p4 GENERIC amd64 ``` ACKs for top commit: maflcko: re-cr-lgtm-ACK 5a0688a jonatack: ACK 5a0688a tested on macOS only theStack: ACK 5a0688a Tree-SHA512: 8c88d282d09c00355d22c4c504b779f60e420327a5e07bcf80fa77b97fefcb04952af9ceaf439d9033a0a2448cb26a02663fe6bddcd4a74792857cfbaf1c5162

5a0688a test: enable reindex readonly test on *BSD and macOS as root (Matthew Zipkin) Pull request description: see bitcoin#27850 (comment) OpenBSD and FreeBSD don't have `chattr` but they do have `chflags`, use that method to make the block file immutable for the reindex_readonly test. Written and tested on a VPS running FreeBSD: ``` FreeBSD freebsd-13-1 13.2-RELEASE-p4 FreeBSD 13.2-RELEASE-p4 GENERIC amd64 ``` ACKs for top commit: maflcko: re-cr-lgtm-ACK 5a0688a jonatack: ACK 5a0688a tested on macOS only theStack: ACK 5a0688a Tree-SHA512: 8c88d282d09c00355d22c4c504b779f60e420327a5e07bcf80fa77b97fefcb04952af9ceaf439d9033a0a2448cb26a02663fe6bddcd4a74792857cfbaf1c5162

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

src/test/blockmanager_tests.cpp Outdated Show resolved Hide resolved

test/functional/feature_reindex.py Outdated Show resolved Hide resolved

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

test/functional/feature_reindex.py Outdated Show resolved Hide resolved

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

jonatack reviewed Sep 5, 2023

View reviewed changes

DrahtBot requested a review from maflcko September 5, 2023 16:58

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

pinheadmz force-pushed the blockstore-tests branch from 9791b31 to f1264f9 Compare September 6, 2023 14:57

jonatack reviewed Sep 7, 2023

View reviewed changes

DrahtBot requested a review from maflcko September 7, 2023 19:20

maflcko approved these changes Sep 14, 2023

View reviewed changes

DrahtBot removed the request for review from achow101 September 14, 2023 15:06

achow101 reviewed Sep 14, 2023

View reviewed changes

test: cover read-only blockstore

de8f912

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

pinheadmz force-pushed the blockstore-tests branch from f1264f9 to de8f912 Compare September 14, 2023 16:02

DrahtBot requested review from achow101 and maflcko September 14, 2023 16:19

maflcko approved these changes Sep 14, 2023

View reviewed changes

test/functional/feature_reindex_readonly.py Show resolved Hide resolved

DrahtBot removed the request for review from achow101 September 14, 2023 17:15

achow101 merged commit 541976b into bitcoin:master Sep 14, 2023

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

bitcoin locked and limited conversation to collaborators Oct 15, 2024

		@@ -0,0 +1,61 @@
		#!/usr/bin/env python3
		# Copyright (c) 2023 The Bitcoin Core developers

	# 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}")

test: Add unit & functional test coverage for blockstore #27850

test: Add unit & functional test coverage for blockstore #27850

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!

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

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!

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Sep 6, 2023

Uh oh!

jonatack left a comment • 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!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Sep 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Sep 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Jun 10, 2023 •

edited

Loading

DrahtBot commented Jun 10, 2023 •

edited

Loading

pinheadmz commented Jun 13, 2023 •

edited

Loading

jonatack Sep 5, 2023 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack commented Sep 14, 2023 •

edited

Loading