test: Add unit & functional test coverage for blockstore

pinheadmz commented at 1:14 pm on June 10, 2023: member

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 commented at 1:14 pm on June 10, 2023: 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
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.

pinheadmz force-pushed on Jun 10, 2023

DrahtBot added the label CI failed on Jun 10, 2023

pinheadmz force-pushed on Jun 10, 2023

in src/test/blockmanager_tests.cpp:97 in 77bf6172a0 outdated

92+        : BasicTestingSetup{ChainType::MAIN, /*extra_args=*/{"-fastprune=1"}} {}
93+};
94+
95+BOOST_FIXTURE_TEST_CASE(blockmanager_flush_block_file, FastPruneTestingSetup)
96+{
97+    const auto params{CreateChainParams(ArgsManager{}, ChainType::MAIN)};

furszy commented at 10:47 pm on June 10, 2023:

In 77bf6172: nit: Could use the existent test params:

0    const auto& params = Params();

pinheadmz commented at 5:20 pm on June 12, 2023:

thanks 👍

pinheadmz force-pushed on Jun 10, 2023

in test/functional/feature_reindex.py:117 in 38bc46316d outdated

114+            # Depending on the filesystem, attempted flushing to the read-only file will either happen...
115+            try:
116+                # ...during initialization...
117+                self.start_node(1, extra_args=["-reindex", "-fastprune"])
118+                # ...or upon shutdown, if initialization succeeds.
119+                self.stop_node(1)

furszy commented at 4:47 pm on June 11, 2023:

~Do you know why the node is flushing stuff to disk when nothing happened?~

Edit: answer: #27039.. so nvm.

pinheadmz marked this as a draft on Jun 11, 2023

dergoegge commented at 10:07 am on June 12, 2023: member

Concept ACK

DrahtBot renamed this:
~~Add unit & functional test coverage for blockstore~~
test: Add unit & functional test coverage for blockstore
on Jun 12, 2023

pinheadmz force-pushed on Jun 12, 2023

in test/functional/feature_reindex.py:117 in e01ac849ef outdated

108+        self.log.debug("Make the first block file read-only")
109+        filename = self.nodes[1].chain_path / 'blocks' / 'blk00000.dat'
110+        os.chmod(filename, stat.S_IREAD)
111+
112+        self.log.debug("Attempt to restart and reindex the node with the read-only block file")
113+        with self.nodes[1].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):

maflcko commented at 9:05 am on June 13, 2023:

shouldn’t the messages be checked by assert_start_raises_init_error instead of reading the debug log?

pinheadmz commented at 2:05 pm on June 13, 2023:

The actual error ends up being generic:

0Error: A fatal internal error occurred, see debug.log for details

and actually before #27708 was merged yesterday, this code was even more messy!

maflcko commented at 9:19 pm on June 13, 2023:

Ah ok, then maybe check Error: A fatal internal error occurred, see debug.log for details as error message to provide context for test readers?

pinheadmz commented at 0:02 am on June 14, 2023:

done, thanks

pinheadmz force-pushed on Jun 13, 2023

pinheadmz commented at 7:27 pm on June 13, 2023: member

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 force-pushed on Jun 14, 2023

pinheadmz marked this as ready for review on Jun 14, 2023

pinheadmz commented at 11:17 am on June 14, 2023: member

Ready for review now, CI is passing modulo https://github.com/bitcoin/bitcoin/issues/27879

willcl-ark commented at 12:07 pm on June 14, 2023: member

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 force-pushed on Jun 14, 2023

pinheadmz commented at 2:13 pm on June 14, 2023: member

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

pinheadmz force-pushed on Jun 14, 2023

pinheadmz commented at 6:15 pm on June 14, 2023: member

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

Narrator: “It didn’t.” https://cirrus-ci.com/build/4907092587315200

Seemed like a good idea though but bitcoind is still able to open the “immutable” files in write mode 🤔

Maybe @MarcoFalke can help us figure out a better method. Doesn’t seem like a terrible idea to run a few CI tasks as a user instead of root? There might be other little behaviors like this one.

pinheadmz force-pushed on Jun 14, 2023

DrahtBot removed the label CI failed on Jun 14, 2023

DrahtBot added the label Tests on Jun 14, 2023

maflcko commented at 6:09 am on June 15, 2023: member

If you want to add a user account for this, you can try reverting #27376

Edit: Sorry, this is not reliable. See #27850 (review)

DrahtBot added the label CI failed on Jun 15, 2023

pinheadmz force-pushed on Jun 17, 2023

pinheadmz force-pushed on Jun 20, 2023

pinheadmz commented at 8:27 pm on June 20, 2023: member

@MarcoFalke

If you want to add a user account for this, you can try reverting #27376

I’ve been banging on this for a few days and I think I see the issue now: since #21619 we set DANGER_RUN_CI_ON_HOST: "1" meaning all the nonroot user stuff I tried adding back in from #27376 gets skipped anyway (because it was all docker configuration and this flag skips docker entirely). I tried just now reverting that single flag which results in every unit crashing with docker not found

So, (cc: @willcl-ark) I can continue down this path of trying to get a non-root user running on CI, or I can go back to “skip test if root” in python.

in test/functional/feature_reindex.py:84 in 8d874ba144 outdated

81         assert_equal(self.nodes[0].getblockcount(), 12)
82 
83+    def reindex_readonly(self):
84+        # Root user overrides read-only file permissions, breaking test (not an issue on Windows).
85+        if os.name != 'nt':
86+            assert os.getuid() != 0

maflcko commented at 6:23 am on June 21, 2023:

On a second thought, I don’t think we can rely (or assert) in the tests that they are not run as root. Anyone should be free to run the tests where they want, also as root.

pinheadmz commented at 2:15 pm on June 21, 2023:

Oh yeah this is obviously an important point. Lots of users probably run bitcoind as root on a VPS and they should be able to run all tests.

pinheadmz force-pushed on Jun 21, 2023

in test/functional/blockstore_reindex.py:48 in 0447b1c8c3 outdated

43+
44+        self.log.debug("Generate enough blocks to start second block file")
45+        self.generatetoaddress(self.nodes[1], blocks_needed, addr)
46+        self.stop_node(1)
47+
48+        assert os.path.exists(self.nodes[1].chain_path / 'blocks' / 'blk00000.dat')

maflcko commented at 4:01 pm on June 21, 2023:

style nit: Can be written shorter and without an include:

0        assert (self.nodes[1].chain_path / "blocks" / "blk00000.dat").exists()

See https://docs.python.org/3/library/pathlib.html#correspondence-to-tools-in-the-os-module

pinheadmz commented at 5:10 pm on June 21, 2023:

awesome thanks

in test/functional/test_runner.py:163 in 0447b1c8c3 outdated

159@@ -160,6 +160,7 @@
160     'wallet_abandonconflict.py --legacy-wallet',
161     'wallet_abandonconflict.py --descriptors',
162     'feature_reindex.py',
163+    'blockstore_reindex.py',

maflcko commented at 4:02 pm on June 21, 2023:

Why not simply call this feature_reindex_readonly to avoid introducing a new category?

pinheadmz commented at 5:10 pm on June 21, 2023:

I’ll revert but I guess I was thinking that the new test isn’t really about the reindexing feature, it’s more like a blockstore unit test

DrahtBot removed the label CI failed on Jun 21, 2023

pinheadmz force-pushed on Jun 21, 2023

pinheadmz commented at 6:19 pm on June 21, 2023: member

push to 28c6a7d5a2d85f5f5b99e21925f9fd966eaa347f addresses @MarcoFalke nits (thank you!) and also adds a commit to run functional tests as a non-root user on CI. This works locally, so fingers crossed it works on actual ci as well.

Note that without that commit, the blockstore test gets skipped: https://cirrus-ci.com/task/5520869955469312?logs=ci#L3423

so we’ll see if it passes nice n clean on this run…

DrahtBot added the label CI failed on Jun 21, 2023

pinheadmz force-pushed on Jun 22, 2023

in ci/test/00_setup_env.sh:42 in 8e13908011 outdated

38@@ -39,6 +39,7 @@ export USE_BUSY_BOX=${USE_BUSY_BOX:-false}
39 export RUN_UNIT_TESTS=${RUN_UNIT_TESTS:-true}
40 export RUN_FUNCTIONAL_TESTS=${RUN_FUNCTIONAL_TESTS:-true}
41 export RUN_TIDY=${RUN_TIDY:-false}
42+export CREATE_NONROOT_USER=${CREATE_NONROOT_USER:-true}

maflcko commented at 2:51 pm on June 22, 2023:

if macos is the only failure, it might be good enough to guard it instead of adding a new global CI setting?

pinheadmz commented at 3:04 pm on June 22, 2023:

Ok gotcha, will update

in ci/test/06_script_b.sh:16 in 8e13908011 outdated

12+if [ "$CREATE_NONROOT_USER" = "true" ]; then
13+  useradd nonroot
14+  usermod -a -G root nonroot
15+  export NONROOT_PREFIX="su nonroot"
16+  export NONROOT_PERMISSIONS="chmod -R g+rwx ${BASE_ROOT_DIR}"
17+fi

maflcko commented at 2:52 pm on June 22, 2023:

nit: Could make sense to call echo > "${HOME}/.bitcoin" for nonroot as well?

pinheadmz commented at 3:04 pm on June 22, 2023:

~sorry, I don’t understand this, echo to a directory?~

aha, I get it, will do: # Make sure default datadir does not exist and is never read by creating a dummy file

maflcko commented at 2:53 pm on June 22, 2023: member

left two nits

DrahtBot removed the label CI failed on Jun 22, 2023

pinheadmz force-pushed on Jun 22, 2023

DrahtBot added the label CI failed on Jun 22, 2023

pinheadmz force-pushed on Jun 22, 2023

DrahtBot removed the label CI failed on Jun 22, 2023

in ci/test/06_script_b.sh:17 in 855e6115e2 outdated

12+if [ "$CI_OS_NAME" != "macos" ]; then
13+  useradd -m nonroot
14+  usermod -a -G root nonroot
15+  export NONROOT_PREFIX="su nonroot"
16+  export NONROOT_PERMISSIONS="chmod -R g+rwx ${BASE_ROOT_DIR}"
17+  # Make sure default datadir does not exist and is never read by creating a dummy file

maflcko commented at 7:28 am on June 23, 2023:

nit: May be good to not duplicate this comment and locate the logic in two different places. I guess you can move-only the other thing up?

pinheadmz commented at 12:33 pm on June 23, 2023:

good call, I’ll repeat the pattern of defining nonroot commands here on top but executing them where its appropriate below…

 0diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
 1index 39a84b4fcf..1e8fc5ade9 100755
 2--- a/ci/test/06_script_b.sh
 3+++ b/ci/test/06_script_b.sh
 4@@ -14,8 +14,7 @@ if [ "$CI_OS_NAME" != "macos" ]; then
 5   usermod -a -G root nonroot
 6   export NONROOT_PREFIX="su nonroot"
 7   export NONROOT_PERMISSIONS="chmod -R g+rwx ${BASE_ROOT_DIR}"
 8-  # Make sure default datadir does not exist and is never read by creating a dummy file
 9-  echo > ~nonroot/.bitcoin
10+  export NONROOT_DUMMY_DIR="echo > ~nonroot/.bitcoin"
11 fi
12 
13 if [ "$CI_OS_NAME" == "macos" ]; then
14@@ -63,6 +62,7 @@ if [ "$CI_OS_NAME" == "macos" ]; then
15   echo > "${HOME}/Library/Application Support/Bitcoin"
16 else
17   echo > "${HOME}/.bitcoin"
18+  ${NONROOT_DUMMY_DIR}
19 fi
20 
21 if [ -z "$NO_DEPENDS" ]; then

pinheadmz force-pushed on Jun 23, 2023

pinheadmz commented at 5:13 pm on July 3, 2023: member

@MarcoFalke @ryanofsky this is ready for review if you have time 🕺

DrahtBot added the label Needs rebase on Jul 6, 2023

pinheadmz force-pushed on Jul 7, 2023

pinheadmz commented at 2:27 pm on July 7, 2023: member

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

Thanks handsome bot! 🍦

DrahtBot removed the label Needs rebase on Jul 7, 2023

pinheadmz force-pushed on Jul 7, 2023

DrahtBot added the label CI failed on Jul 7, 2023

DrahtBot removed the label CI failed on Jul 7, 2023

pinheadmz requested review from maflcko on Jul 10, 2023

pinheadmz requested review from furszy on Jul 10, 2023

in src/test/blockmanager_tests.cpp:99 in 2b624780b6 outdated

90@@ -90,4 +91,71 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain
91     BOOST_CHECK(!AutoFile(blockman.OpenBlockFile(new_pos, true)).IsNull());
92 }
93 
94+struct FastPruneTestingSetup : public BasicTestingSetup {
95+    FastPruneTestingSetup()
96+        : BasicTestingSetup{ChainType::MAIN, /*extra_args=*/{"-fastprune=1"}} {}
97+};
98+
99+BOOST_FIXTURE_TEST_CASE(blockmanager_flush_block_file, FastPruneTestingSetup)

ryanofsky commented at 2:35 pm on July 10, 2023:

In commit “init: start indexes sync earlier” (ed4462cc78afd2065bbf5bd79728852b65b9ad7f)

Nice test. Blockman API is really confusing but the comments here make everything very clear, and having the test should make improvements easier to understand, and help ensure things don’t get broken unintentionally.

ryanofsky approved

ryanofsky commented at 3:01 pm on July 10, 2023: contributor

Code review ACK ca91c244ef1ba7eac6643d66a5fc56d3a2a8b550. The new tests seem useful and are very easy to read, and I think switching CI to run as non-root is a nice change that should make CI environment more useful and realistic.

I do think the instinct to skip tests is a mistake, though. Usually better to just let tests run in any environment and check whatever the expected behavior is in that environment, unless the behavior is really unstable or nondeterministic. In this case, rather than skipping the test as root, I think it would be slightly better to let the test run as root, and check that there isn’t an error, to make sure some other bug doesn’t happen that could go undetected. But this is not very important, and current approach seems ok, too.

DrahtBot added the label Needs rebase on Jul 10, 2023

in src/test/blockmanager_tests.cpp:97 in 2b624780b6 outdated

90@@ -90,4 +91,71 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain
91     BOOST_CHECK(!AutoFile(blockman.OpenBlockFile(new_pos, true)).IsNull());
92 }
93 
94+struct FastPruneTestingSetup : public BasicTestingSetup {
95+    FastPruneTestingSetup()
96+        : BasicTestingSetup{ChainType::MAIN, /*extra_args=*/{"-fastprune=1"}} {}
97+};

maflcko commented at 9:39 am on July 13, 2023:

2b624780b601be60700a27f893d24c0e8a11cd41: Can remove this struct, no? Otherwise I’d like to understand why it is needed and why it works. The -fastprune=1 you pass isn’t picked up by blockman_opts.

pinheadmz commented at 11:52 am on July 19, 2023:

thanks, that is leftover from older version

in test/functional/feature_reindex_readonly.py:31 in d82a493ed9 outdated

44+        self.generatetoaddress(self.nodes[1], blocks_needed, addr)
45+        self.stop_node(1)
46+
47+        assert (self.nodes[1].chain_path / "blocks" / "blk00000.dat").exists()
48+        assert (self.nodes[1].chain_path / "blocks" / "blk00001.dat").exists()
49+

maflcko commented at 9:58 am on July 13, 2023:

d82a493ed9570e36d45ce0619691f958fad8429a: Agree with ryan that this can be run on root as well, also the function skip_if_root seems confusing, due to the windows handling (at least to me).

Might be better to just put a if (root...): guard in this line in this file only?

pinheadmz commented at 12:39 pm on July 19, 2023:

yeah agreed, will update

in test/functional/feature_reindex_readonly.py:35 in d82a493ed9 outdated

33+        block_hash = self.generatetoaddress(self.nodes[1], 1, addr)[0]
34+        block_size = self.nodes[1].getblock(block_hash)["size"]
35+        block_size += 8 # BLOCK_SERIALIZATION_HEADER_SIZE
36+
37+        # How many blocks do we need to roll over a new .blk file?
38+        block_count = self.nodes[1].getblockcount()

maflcko commented at 9:59 am on July 13, 2023:

Might be better to just assert this is 1, or omit, as otherwise it would lead to issues anyway?

pinheadmz commented at 1:11 pm on July 19, 2023:

ok will assert so future failure on regression is easy to see

in test/functional/feature_reindex_readonly.py:24 in d82a493ed9 outdated

22+    def skip_test_if_missing_module(self):
23+        self.skip_if_root()
24+
25+    def reindex_readonly(self):
26+        self.connect_nodes(0, 1)
27+        addr = self.nodes[1].get_deterministic_priv_key().address

maflcko commented at 10:02 am on July 13, 2023:

Any reason to use this, as it’s length may change and break the test? I wonder if the test runs faster if you just bloat each block with 1MB of nulldata?

Something like: d=999*"ff";generateblock(output=f"raw(6a{d})", tx=[]);?

pinheadmz commented at 1:21 pm on July 19, 2023:

Hm well, that’ll certainly be faster but then we can’t verify a successful reindex in the new run-as-root path. Size shouldn’t be an issue though because no assumptions are made about it, the test calculates how much data it needs to generate. Let me know what you think about this again after the next push

maflcko commented at 3:14 pm on July 20, 2023:

that’ll certainly be faster but then we can’t verify a successful reindex in the new run-as-root path.

Any reason why it wouldn’t be able to reindex?

pinheadmz commented at 3:18 pm on July 20, 2023:

ah ya know what I didn’t read your comment correctly I thought you were suggesting filling the blocks with invalid junk but that line with generateblock() is cool

maflcko commented at 5:02 pm on July 20, 2023:

So I just tried this and you don’t need to generate any additional blocks. A single call to self.generateblock(self.nodes[1], output=f"raw(61{'ff'*998000})", transactions=[]) is enough to create blk2.dat

pinheadmz commented at 5:03 pm on July 20, 2023:

yeah, ive got this too, will push in a few minutes

in ci/test/06_script_b.sh:65 in fe9da891eb outdated

61@@ -53,6 +62,7 @@ if [ "$CI_OS_NAME" == "macos" ]; then
62   echo > "${HOME}/Library/Application Support/Bitcoin"
63 else
64   echo > "${HOME}/.bitcoin"
65+  ${NONROOT_DUMMY_DIR}

maflcko commented at 10:04 am on July 13, 2023:

nit in fe9da891eb7a7440b6c23d63e129525fe26f8f0e: Can just inline echo > ~nonroot/.bitcoin directly?

pinheadmz commented at 2:44 pm on July 19, 2023:

hm see #27850 (review) not sure what to do

ryanofsky commented at 2:16 pm on July 20, 2023:

re: #27850 (review)

hm see #27850 (comment) not sure what to do

I don’t actually understand what this issue is in that comment since the variable seems to be used just once.

It does seem strange to define these commands as variables instead of inlining them when the variables only seem to be used only once. Also would expect them to be shell functions instead of variables. And would expect them to have imperative names like NONROOT_MAKE_DUMMY_DIR instead of NONROOT_DUMMY_DIR. But I guess a nice thing about having the variables is it ties nonroot code scattered different places back to a central place. Anyway, whatever style you like to use here seems fine.

maflcko commented at 5:07 pm on July 20, 2023:

My understanding is that inlining should be fine here, because if the previous nonroot code was removed, the call would fail with “path not found”, does it not?

edit: But anything is fine here.

maflcko approved

maflcko commented at 10:07 am on July 13, 2023: member

lgtm ACK fe9da891eb7a7440b6c23d63e129525fe26f8f0e 🚊

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK fe9da891eb7a7440b6c23d63e129525fe26f8f0e 🚊
38QzK0cCQfezOzJ9M4z6wqlTMaWEONVg40/EewcQ6XoeRcHwSTAsYNubGtq6tvqdOQbK2cyGUwMhQwOjiq4dqBQ==

DrahtBot requested review from ryanofsky on Jul 13, 2023

in test/functional/feature_reindex_readonly.py:39 in fe9da891eb outdated

36+
37+        # How many blocks do we need to roll over a new .blk file?
38+        block_count = self.nodes[1].getblockcount()
39+        fastprune_blockfile_size = 0x10000
40+        size_needed = fastprune_blockfile_size - (block_size * block_count)
41+        blocks_needed = size_needed // block_size

maflcko commented at 10:08 am on July 13, 2023:

nit: Shouldn’t this use ceil instead of floor?

pinheadmz commented at 2:38 pm on July 19, 2023:

Boy I was really scratching my head over this for like an hour (“how does this work at all?”) then I remember the genesis block is written to a file when block count is still 0. So that’s how we are rounding up.

in src/test/blockmanager_tests.cpp:138 in 2b624780b6 outdated

133+
134+    // First two blocks are written as expected
135+    CBlock read_block;
136+    blockman.ReadBlockFromDisk(read_block, pos1);
137+    BOOST_CHECK_EQUAL(read_block.nVersion, 1);
138+    blockman.ReadBlockFromDisk(read_block, pos2);

furszy commented at 2:13 pm on July 18, 2023:

In 2b624780:

nit: could also check the ReadBlockFromDisk bool return value.

pinheadmz commented at 11:53 am on July 19, 2023:

sure thanks

in src/test/blockmanager_tests.cpp:149 in 2b624780b6 outdated

144+    // when the user has the blocks on disk but the metadata is being rebuilt.
145+    // Verify this behavior by attempting (and failing) to write block 3 data
146+    // to block 2 location.
147+    CBlockFileInfo* block_data = blockman.GetBlockFileInfo(0);
148+    BOOST_CHECK_EQUAL(block_data->nBlocks, 2);
149+    blockman.SaveBlockToDisk(block3, /*nHeight=*/3, chain, /*dbp=*/&pos2);

furszy commented at 2:28 pm on July 18, 2023:

In 2b62478:

nit: could also check that SaveBlockToDisk return value is equal to pos2.

Side note: This behavior smells bad to me. Having SaveBlockToDisk updating the block file metadata, without storing the block, and returning successfully looks like an error-prone behavior. The block file info class exist to describe what is inside the block file.

pinheadmz commented at 12:08 pm on July 19, 2023:

done thanks. agreed about the smell. lets lock down the expected behavior with tests and then we can clean up in future PR.

furszy commented at 2:48 pm on July 18, 2023: member

Code ACK 2b624780

The introduced unit test documents what I see as an error-prone behavior that would be nice to improve in the future: The BlockFileInfo class could be storing information that is not related to the block file it describes.

DrahtBot requested review from furszy on Jul 18, 2023

maflcko commented at 6:02 pm on July 18, 2023: member

Are you still working on this?

pinheadmz commented at 6:03 pm on July 18, 2023: member

Are you still working on this?

Absolutely, thanks. Had a few other things to catch up on. This will be updated before the end of the week

pinheadmz force-pushed on Jul 19, 2023

in src/test/blockmanager_tests.cpp:177 in 9fd44f813f outdated

172+    BOOST_CHECK(!blockman.ReadBlockFromDisk(read_block, pos1));
173+    ASSERT_DEBUG_LOG("ReadBlockFromDisk: Errors in block header");
174+    BOOST_CHECK_EQUAL(read_block.nVersion, 1);
175+    BOOST_CHECK(!blockman.ReadBlockFromDisk(read_block, pos2));
176+    ASSERT_DEBUG_LOG("ReadBlockFromDisk: Errors in block header");
177+    BOOST_CHECK_EQUAL(read_block.nVersion, 2);

furszy commented at 3:18 pm on July 19, 2023:

Need to scope the ASSERT_DEBUG_LOG macro. It expands to a DebugLogHelper, which has the check in the destructor. Also, this will swallow other logging errors as well if it is not scoped.

pinheadmz commented at 4:30 pm on July 19, 2023:

Thanks, just fixed & pushed

DrahtBot removed the label Needs rebase on Jul 19, 2023

pinheadmz force-pushed on Jul 19, 2023

pinheadmz commented at 1:33 pm on July 20, 2023: member

@MarcoFalke @furszy @ryanofsky thanks for the reviews, latest push addresses all comments except where noted.

ryanofsky approved

ryanofsky commented at 2:22 pm on July 20, 2023: contributor

Code review ACK 08b275c6d27f06cd7d2580b4c0200e32d5b7b1b6. This adds nice clear tests and a more realistic CI environment.

DrahtBot requested review from furszy on Jul 20, 2023

DrahtBot requested review from maflcko on Jul 20, 2023

maflcko commented at 3:15 pm on July 20, 2023: member

lgtm ACK 08b275c6d27f06cd7d2580b4c0200e32d5b7b1b6 🥛

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 08b275c6d27f06cd7d2580b4c0200e32d5b7b1b6 🥛
3H6fyAbnBh5wUC7aDiz0JV0xtCSZwQhLf06w6lLmL7uypVzV7p0SxdsIsxZtMiUoaA3+XmszUKfNPZDxRUD+EAw==

DrahtBot removed review request from maflcko on Jul 20, 2023

in test/functional/feature_reindex_readonly.py:18 in 08b275c6d2 outdated

13+class BlockstoreReindexTest(BitcoinTestFramework):
14+    def set_test_params(self):
15+        self.setup_clean_chain = True
16+        self.num_nodes = 2
17+        self.extra_args = [
18+            [],

maflcko commented at 5:04 pm on July 20, 2023:

Any reason for this node?

pinheadmz commented at 5:13 pm on July 20, 2023:

nope leftover from last iteration sorry, removing

pinheadmz force-pushed on Jul 20, 2023

DrahtBot added the label CI failed on Jul 20, 2023

maflcko approved

maflcko commented at 6:02 pm on July 20, 2023: member

lgtm ACK 068b523ee7720e6a392efc294ff89bf936a3e2ac 🍈

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 068b523ee7720e6a392efc294ff89bf936a3e2ac  🍈
3bOm6LDsPGmuAHl7JYbkFQ832I+ZYkRpkK8HygGqEhGUdNPoYuTHY5kg66wEYAMuTzkx5oZEyEoqcHQA8QkXXAg==

DrahtBot requested review from ryanofsky on Jul 20, 2023

in test/functional/test_framework/test_framework.py:940 in 068b523ee7 outdated

935@@ -936,6 +936,9 @@ def skip_if_no_external_signer(self):
936         if not self.is_external_signer_compiled():
937             raise SkipTest("external signer support has not been compiled.")
938 
939+    def is_unix_root(self):
940+        return os.name != 'nt' and os.getuid() == 0

jonatack commented at 7:22 pm on July 20, 2023:

It looks like the PR description section about skip_if_root is now out of date.

pinheadmz commented at 7:32 pm on July 20, 2023:

thank you, fixed

in test/functional/feature_reindex_readonly.py:43 in 068b523ee7 outdated

38+            assert_equal(self.nodes[0].getblockcount(), block_count)
39+        else:
40+            self.log.debug("Running as non-root user, will fail opening read-only file")
41+            with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):
42+                self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
43+                    expected_msg="Error: A fatal internal error occurred, see debug.log for details")

jonatack commented at 7:23 pm on July 20, 2023:

If you retouch, I think the logging for the assertions inside this is_unix_root conditional should be logged as info (maybe all the debug logging in this file currently could be info).

jonatack commented at 7:31 pm on July 20, 2023: member

ACK 068b523ee7720e6a392efc294ff89bf936a3e2ac

didn’t review the CI and is_unix_root commits -> tested those commits and based #28116 on them (very helpful, good job!)

DrahtBot removed the label CI failed on Jul 20, 2023

furszy commented at 1:34 pm on July 25, 2023: member

Code review ACK 068b523e, except the CI changes.

achow101 commented at 9:42 pm on July 25, 2023: member

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

Narrator: “It didn’t.” https://cirrus-ci.com/build/4907092587315200

Seemed like a good idea though but bitcoind is still able to open the “immutable” files in write mode thinking

I’ve just tried doing that locally and the test seems to have worked. The error shows up in the log as expected. Maybe it’s just an issue with the larger test that it was part of at the time of that commit?

Personally, I’d rather not need to change the CI environment like this just to run this test.

jonatack commented at 9:51 pm on July 25, 2023: member

@achow101 I found the CI environment changes useful for making #28116 work as well (to take the review feedback in #28116 (review)) and based it on them.

in ci/test/06_script_b.sh:160 in 068b523ee7 outdated

156@@ -144,7 +157,7 @@ if [ "$RUN_UNIT_TESTS_SEQUENTIAL" = "true" ]; then
157 fi
158 
159 if [ "$RUN_FUNCTIONAL_TESTS" = "true" ]; then
160-  bash -c "LD_LIBRARY_PATH=${DEPENDS_DIR}/${HOST}/lib ${TEST_RUNNER_ENV} test/functional/test_runner.py --ci $MAKEJOBS --tmpdirprefix ${BASE_SCRATCH_DIR}/test_runner/ --ansi --combinedlogslen=99999999 --timeout-factor=${TEST_RUNNER_TIMEOUT_FACTOR} ${TEST_RUNNER_EXTRA} --quiet --failfast"
161+  $NONROOT_PREFIX bash -c "LD_LIBRARY_PATH=${DEPENDS_DIR}/${HOST}/lib ${TEST_RUNNER_ENV} test/functional/test_runner.py --ci $MAKEJOBS --tmpdirprefix ${BASE_SCRATCH_DIR}/test_runner/ --ansi --combinedlogslen=99999999 --timeout-factor=${TEST_RUNNER_TIMEOUT_FACTOR} ${TEST_RUNNER_EXTRA} --quiet --failfast"

maflcko commented at 10:03 am on July 26, 2023:

Apologies for missing this. It is an undocumented requirement that the functional usdt tests are run as root (not sure why). So this will disable them.

I think this should either be reverted, or the usdt requirement for root be dropped.

pinheadmz commented at 4:01 pm on July 26, 2023:

I think this should either be reverted, or the usdt requirement for root be dropped.

How would you feel about adding an environment variable to “force” root in 00_setup_env_native_asan.sh? The functional test I added no longer skips if root (just expects a different behavior), so this actually would be kinda nice to have one task does run all functional tests as root.

attempting in 19b6353d3866769ea84b7759396078745c0a8ecf

pinheadmz commented at 6:57 pm on July 26, 2023:

looks good, runs usdt test: https://cirrus-ci.com/task/4683666694078464?logs=ci#L4600-L4604

achow101 commented at 8:59 pm on July 28, 2023:

It is an undocumented requirement that the functional usdt tests are run as root (not sure why).

For whatever reason, USDTs don’t work for non-root users.

maflcko changes_requested

maflcko commented at 6:12 am on July 27, 2023: member

I think you’ll also have to reply to the review comment #27850 (comment)

pinheadmz commented at 5:23 pm on July 27, 2023: member

I’ve just tried doing that locally and the test seems to have worked. The error shows up in the log as expected. Maybe it’s just an issue with the larger test that it was part of at the time of that commit?

see #28171 rebased on master without any changes to CI framework. Using chattr +i does not prevent the root user from opening an “immutable” file in write mode in this task and some other permission issue in this one

maflcko commented at 5:26 pm on July 27, 2023: member

The first issue is inside a container:

0chattr: Operation not permitted while setting flags on /tmp/cirrus-build-3580104950/ci/scratch/test_runner/test_runner_₿_🏃_20230727_161727/feature_reindex_readonly_206/node0/regtest/blocks/blk00000.dat

The second issue is that the file can’t be deleted, because you’ll have to grant/undo the permission again before the end of the test.

maflcko commented at 5:27 pm on July 27, 2023: member

https://duckduckgo.com/?q=chattr+inside+docker should fix it

pinheadmz commented at 8:14 pm on July 28, 2023: member

https://duckduckgo.com/?q=chattr+inside+docker should fix it

We’re still having trouble getting this to work: https://github.com/achow101/bitcoin/commits/28171-chattr-fixes https://cirrus-ci.com/build/5095863295410176

achow101 commented at 8:58 pm on July 28, 2023: member

ACK 19b6353d3866769ea84b7759396078745c0a8ecf

Although the chattr approach worked locally, we couldn’t figure out how to make it work in CI, so the approach of running CI as a non-root user seems reasonable.

DrahtBot requested review from furszy on Jul 28, 2023

DrahtBot requested review from jonatack on Jul 28, 2023

DrahtBot requested review from maflcko on Jul 28, 2023

maflcko commented at 1:27 pm on July 29, 2023: member

We’ll be dropping support for Cirrus CI dockerfiles anyway within the next 30 days, going to full VMs (either on GitHub Actions CI, or on self-hosted machines), so I think chattr should work fine, if this pull can wait for a few more days.

pinheadmz commented at 1:46 pm on July 29, 2023: member

Cool. Is there an open PR for that yet? Plz add me as reviewer!

maflcko commented at 2:39 pm on July 29, 2023: member

Sure, #28161 is the fist step. It won’t make a difference for chattr, because it is just moving one VM to another VM, but it is the first step.

maflcko commented at 10:10 am on July 30, 2023: member

(Feel free to push the chattr changes here in the meantime. Obviously the CI won’t pass, but people can start initial review)

maflcko commented at 10:35 am on August 4, 2023: member

#28214 is the second step

DrahtBot added the label CI failed on Aug 11, 2023

maflcko commented at 2:28 pm on August 14, 2023: member

Further steps require a bugfix: https://github.com/bitcoin/bitcoin/pull/28185

maflcko added the label Needs rebase on Aug 14, 2023

DrahtBot removed the label Needs rebase on Aug 14, 2023

DrahtBot added the label Needs rebase on Aug 17, 2023

maflcko commented at 2:19 pm on August 17, 2023: member

The final step is #21652. Feel free to rebase on top of that, to test your changes.

pinheadmz force-pushed on Aug 22, 2023

pinheadmz commented at 6:52 pm on August 22, 2023: member

The final step is #21652. Feel free to rebase on top of that, to test your changes.

trying this now!

DrahtBot removed the label Needs rebase on Aug 22, 2023

pinheadmz commented at 8:00 pm on August 22, 2023: member

the PR’s functional test failed on windows CI but I can’t immediately see why. Note that even though there are fatal errors in the log, those are expected and a successful run of the test would also look pretty much the same…

maflcko commented at 8:55 am on August 23, 2023: member

You can find the error in the logs:

02023-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
12023-08-22T19:24:20.156000Z TestFramework (INFO): Stopping nodes
22023-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
32023-08-22T19:24:20.156000Z TestFramework (INFO): Tests successful
4stderr:
5Usage: chattr [-RVfHv] [+-=mode]... [file]...
6Try 'chattr --help' for more information

pinheadmz commented at 10:24 am on August 23, 2023: member

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:

02023-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 commented at 10:32 am on August 23, 2023: member

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

See

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

pinheadmz force-pushed on Aug 23, 2023

maflcko commented at 7:42 am on August 24, 2023: member

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

Maybe need to install it?

pinheadmz force-pushed on Aug 25, 2023

pinheadmz commented at 5:55 pm on August 25, 2023: member

Maybe need to install it?

Added e2fsprogs in centos containers with force-push

DrahtBot removed the label CI failed on Aug 25, 2023

in ci/test/01_base_install.sh:23 in b00971f120 outdated

19@@ -20,7 +20,7 @@ if [ -n "$DPKG_ADD_ARCH" ]; then
20 fi
21 
22 if [[ $CI_IMAGE_NAME_TAG == *centos* ]]; then
23-  bash -c "dnf -y install epel-release"
24+  bash -c "dnf -y install epel-release e2fsprogs"

maflcko commented at 6:41 am on August 26, 2023:

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.

pinheadmz commented at 11:14 am on August 26, 2023:

good idea, done

maflcko approved

maflcko commented at 6:42 am on August 26, 2023: member

lgtm, thanks for taking the feedback so far

pinheadmz force-pushed on Aug 26, 2023

pinheadmz commented at 11:15 am on August 26, 2023: member

lgtm, thanks for taking the feedback so far

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

pinheadmz force-pushed on Aug 26, 2023

DrahtBot added the label CI failed on Aug 26, 2023

DrahtBot removed the label CI failed on Aug 26, 2023

in test/functional/feature_reindex_readonly.py:26 in 6cda1acdea outdated

21+        self.log.debug("Generate block big enough to start second block file")
22+        fastprune_blockfile_size = 0x10000
23+        opreturn = "6a"
24+        nulldata = fastprune_blockfile_size * "ff"
25+        self.generateblock(self.nodes[0], output=f"raw({opreturn}{nulldata})", transactions=[])
26+        self.nodes[0].getblockcount()

maflcko commented at 5:02 pm on August 27, 2023:

style nit in 6cda1acdea: Can remove this?

pinheadmz commented at 4:00 pm on September 1, 2023:

thanks fixed

in test/functional/feature_reindex_readonly.py:37 in 6cda1acdea outdated

32+        self.log.debug("Make the first block file read-only")
33+        (self.nodes[0].chain_path / "blocks" / "blk00000.dat").chmod(stat.S_IREAD)
34+
35+        filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"
36+        used_chattr = False
37+        if platform.system() == "Linux":

maflcko commented at 5:08 pm on August 27, 2023:

Do you recall why this passes on macOS CI? IIUC it runs under root, so chmod should fail to mark it read-only, no?

If chattr is available on macOS and works, this can be changed to just exclude Windows, with a comment to explain that it uses a different syntax for chattr. Or the syntax can just be adjusted for Windows?

jonatack commented at 5:22 pm on August 27, 2023:

On macOS 13.5.1 (arm64)

0TestFramework (WARNING): Can not make file immutable with chattr, trying chmod instead

maflcko commented at 5:28 pm on August 27, 2023:

On macOS 13.5.1 (arm64)

0TestFramework (WARNING): Can not make file immutable with chattr, trying chmod instead

Yes, I am aware that platform.system() == "Linux" is false on macOS. However, my question was why it passes, and if chattr can be used on macOS.

jonatack commented at 5:32 pm on August 27, 2023:

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

(From what I see searching online, chattr looks to be Linux only.)

maflcko commented at 5:37 pm on August 27, 2023:

Ah yeah, I could have read the second section of https://en.wikipedia.org/wiki/Chattr:

Most BSD-like systems, including macOS, have always had an analogous chflags command to set the attributes, but no command specifically meant to display them; specific options to the ls command are used instead. The chflags command first appeared in 4.4BSD.

pinheadmz commented at 4:06 pm on September 1, 2023:

Yeah there is no chattr on macOS and also on macOS (as far as I’ve been able to tell) root user doesn’t actually have the permission to override permissions:

macOS

0--> touch x
1--> chmod 0 x
2--> echo "!" > x
3-bash: x: Permission denied
4--> sudo echo "!" > x
5-bash: x: Permission denied
6--> chmod 777 x
7--> echo "!" > x
8--> cat x
9!

linux

0# touch x
1# chmod 0 x
2# echo "!" > x
3# cat x
4!

maflcko commented at 5:10 pm on August 27, 2023: member

Only changes since last review:

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

maflcko approved

maflcko commented at 5:10 pm on August 27, 2023: member

left two nits/questions.

re-ACK 6cda1acdea2b908345406389baaef593ac5cce76 🤼

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 6cda1acdea2b908345406389baaef593ac5cce76 🤼
3dakRHCgn0dA/yjlDWocN/nVHFZ2qhu24JHr3Kf+ETosirJP1raLsjYzRUHm5kFp04VPWj0OXABmzLNxYXMymCA==

DrahtBot requested review from achow101 on Aug 27, 2023

DrahtBot requested review from jonatack on Aug 27, 2023

in test/functional/feature_reindex_readonly.py:45 in 6cda1acdea outdated

40+                used_chattr = True
41+            except Exception:
42+                pass
43+
44+        if not used_chattr:
45+            self.log.warning(f"Can not make file immutable with chattr, trying chmod instead")

jonatack commented at 5:58 pm on August 27, 2023:

0            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.

pinheadmz commented at 4:08 pm on September 1, 2023:

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

jonatack commented at 1:19 am on October 1, 2023:

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.

DrahtBot added the label Needs rebase on Aug 30, 2023

pinheadmz force-pushed on Sep 1, 2023

DrahtBot removed the label Needs rebase on Sep 1, 2023

pinheadmz force-pushed on Sep 1, 2023

pinheadmz force-pushed on Sep 5, 2023

pinheadmz commented at 1:10 pm on September 5, 2023: member

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

in src/test/blockmanager_tests.cpp:172 in 9791b31825 outdated

166+    BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), (TEST_BLOCK_SIZE + BLOCK_SERIALIZATION_HEADER_SIZE) * 2);
167+
168+    // First two blocks are written as expected
169+    // Errors are expected because block data is junk, thrown AFTER successful read
170+    CBlock read_block;
171+    {

jonatack commented at 4:42 pm on September 5, 2023:

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.

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

in test/functional/feature_reindex_readonly.py:44 in 9791b31825 outdated

39+                used_chattr = True
40+            except Exception:
41+                pass
42+
43+        if not used_chattr:
44+            self.log.info("Can not make file immutable with chattr, trying chmod instead")

jonatack commented at 4:44 pm on September 5, 2023:

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

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

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

maflcko commented at 7:44 am on September 6, 2023:

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.

jonatack commented at 4:58 pm on September 5, 2023: member

ACK 9791b3182514a2571a7c1cdb38b59338c24d974d

DrahtBot requested review from maflcko on Sep 5, 2023

in ci/test/04_install.sh:35 in cdfb686bd5 outdated

30@@ -31,7 +31,9 @@ if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then
31   fi
32 
33   # shellcheck disable=SC2086
34-  CI_CONTAINER_ID=$(docker run $CI_CONTAINER_CAP --rm --interactive --detach --tty \
35+
36+

maflcko commented at 7:46 am on September 6, 2023:

why two

newlines?

maflcko commented at 7:35 am on September 14, 2023:

nit: I think this wasn’t addressed?

in test/functional/feature_reindex_readonly.py:33 in 9791b31825 outdated

29+        assert (self.nodes[0].chain_path / "blocks" / "blk00001.dat").exists()
30+
31+        self.log.debug("Make the first block file read-only")
32+        (self.nodes[0].chain_path / "blocks" / "blk00000.dat").chmod(stat.S_IREAD)
33+
34+        filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"

maflcko commented at 7:51 am on September 6, 2023:

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

in test/functional/feature_reindex_readonly.py:45 in 9791b31825 outdated

40+            except Exception:
41+                pass
42+
43+        if not used_chattr:
44+            self.log.info("Can not make file immutable with chattr, trying chmod instead")
45+            (self.nodes[0].chain_path / "blocks" / "blk00000.dat").chmod(stat.S_IREAD)

maflcko commented at 7:52 am on September 6, 2023:

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

in test/functional/feature_reindex_readonly.py:54 in 9791b31825 outdated

49+            self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
50+                expected_msg="Error: A fatal internal error occurred, see debug.log for details")
51+
52+        if used_chattr:
53+            subprocess.check_call(['chattr', '-i', filename])
54+        else:

maflcko commented at 7:53 am on September 6, 2023:

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.

in test/functional/feature_reindex_readonly.py:2 in 9791b31825 outdated

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

maflcko commented at 7:54 am on September 6, 2023:

0# 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.

maflcko approved

maflcko commented at 7:56 am on September 6, 2023: member

re-ACK 9791b3182514a2571a7c1cdb38b59338c24d974d 🕕

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 9791b3182514a2571a7c1cdb38b59338c24d974d 🕕
3Yj2uRwzpT1GD2q/ibV1KkO+mrHtJd2SSWN8t4DWILeAQrpp52zK2SWHQFhk/iD04RqlyGEtI32q3lRbj3NNvBg==

unit test: add coverage for BlockManager e573f24202

ci: enable chattr +i capability inside containers 5c2185b3b6

pinheadmz force-pushed on Sep 6, 2023

pinheadmz commented at 6:04 pm on September 6, 2023: member

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

jonatack commented at 7:20 pm on September 7, 2023: member

ACK f1264f9baca4207c4c530f01d90fcdc841368126

DrahtBot requested review from maflcko on Sep 7, 2023

maflcko approved

maflcko commented at 7:36 am on September 14, 2023: member

re-ACK f1264f9baca4207c4c530f01d90fcdc841368126 🍦

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK f1264f9baca4207c4c530f01d90fcdc841368126 🍦
3G5HM6LhMQyxLvXfOSzHbPsljWaZ9XpmswhOHqpwakw2n06WWG+7NyED+63/6l0Lcy6tPvu6axWBFhHvMZKAMAA==

achow101 commented at 3:06 pm on September 14, 2023: member

ACK f1264f9baca4207c4c530f01d90fcdc841368126

DrahtBot removed review request from achow101 on Sep 14, 2023

test: cover read-only blockstore

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

de8f9123af

in test/functional/feature_reindex_readonly.py:43 in f1264f9bac outdated

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

achow101 commented at 3:29 pm on September 14, 2023:

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.

0            try:
1                subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
2                used_chattr = True
3                self.log.info("Made file immutable with chattr")
4            except subprocess.CalledProcessError as e:
5                self.log.warning(str(e))
6                if e.stdout:
7                    self.log.warning(f"stdout: {e.stdout}")
8                if e.stderr:
9                    self.log.warning(f"stderr: {e.stderr}")

pinheadmz commented at 4:02 pm on September 14, 2023:

Added! thanks for the patch

pinheadmz force-pushed on Sep 14, 2023

jonatack commented at 4:19 pm on September 14, 2023: member

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

DrahtBot requested review from achow101 on Sep 14, 2023

DrahtBot requested review from maflcko on Sep 14, 2023

in test/functional/feature_reindex_readonly.py:47 in de8f9123af

42+            except subprocess.CalledProcessError as e:
43+                self.log.warning(str(e))
44+                if e.stdout:
45+                    self.log.warning(f"stdout: {e.stdout}")
46+                if e.stderr:
47+                    self.log.warning(f"stderr: {e.stderr}")

maflcko commented at 4:25 pm on September 14, 2023:

0                self.log.warning(f"{str(e)}\nstdout: {e.stdout}\nstderr: {e.stderr}")

nit: Can be written shorter

(only if you retouch for other reasons)

maflcko commented at 2:33 pm on September 27, 2023:

Also, I think you’ll have to return early in this block.

If running CI, and this fails, the only reason can be that the container has missing capabilities. This can not be worked around (CI is running as root, so chmod doesn’t work)

maflcko commented at 2:49 pm on September 27, 2023:

Fixed in https://github.com/bitcoin/bitcoin/pull/28545

maflcko approved

maflcko commented at 4:25 pm on September 14, 2023: member

lgtm ACK de8f9123afbecc3b4f59fa80af8148bc865d0588 📶

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK de8f9123afbecc3b4f59fa80af8148bc865d0588 📶
3aCT7KL7/lymQNqrfkzsZyKFtz7sH6ZgYVwranWjN6pDe/m5Cd47wdd2X8+yokXAAQkqj7TMxrlrauXR514LfBw==

achow101 commented at 5:15 pm on September 14, 2023: member

ACK de8f9123afbecc3b4f59fa80af8148bc865d0588

DrahtBot removed review request from achow101 on Sep 14, 2023

achow101 merged this on Sep 14, 2023

achow101 closed this on Sep 14, 2023

Frank-GER referenced this in commit be40923930 on Sep 19, 2023

in test/functional/feature_reindex_readonly.py:37 in de8f9123af

32+        self.log.debug("Make the first block file read-only")
33+        filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"
34+        filename.chmod(stat.S_IREAD)
35+
36+        used_chattr = False
37+        if platform.system() == "Linux":

maflcko commented at 11:05 am on October 7, 2023:

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

maflcko commented at 11:05 am on October 7, 2023:

A solution may be to check for freebsd and chflags?

pinheadmz commented at 3:34 pm on October 16, 2023:

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

fanquake referenced this in commit ab61087a7e on Oct 24, 2023

Frank-GER referenced this in commit 425f16a1d3 on Nov 28, 2023

bitcoin locked this on Oct 15, 2024

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

Reviews

Conflicts

macOS

linux