rpc/blockchain: Reset scantxoutset progress before inferring descriptors

[prusnak]

Fixes #19361 by moving resetting the g_scan_progress variable before inferring the descriptors

[promag]

Code review ACK 71fee9c.

There's still a slight chance of getting non zero progress. See b6b1945 for an alternative.

IMO could be backport.

[prusnak]

At first sight, it seems that your patch b6b1945 makes more sense.

[luke-jr]

Still a race, but I don't see an easy fix and this is probably good enough

utACK

[luke-jr]

Oh, @promag's solution does look better though.

[prusnak]

I replaced my commit with the change suggested @promag in 8c4129b

[promag]

ACK

[luke-jr]

If this were to fail, wouldn't it likely block all future reserves?

[promag]

You mean this should be fatal?

[luke-jr]

Since it should be impossible, maybe simplest solution is to just remove it.

Otherwise, it might need something like:

Suggested change

	CHECK_NONFATAL(g_scan_progress == 0);
	if (g_scan_progress) {
	error("ERROR: g_scan_progress was %s when it should be 0, in %s", g_scan_progress, __func__);
	g_scan_progress = 0;
	}

Or:

Suggested change

	CHECK_NONFATAL(g_scan_progress == 0);
	if (g_scan_progress) {
	g_scan_in_progress = false;
	CHECK_NONFATAL(g_scan_progress == 0);
	}

[promag]

Then this should be assert(g_scan_progress == 0). I don't think all RPC code should use CHECK_NONFATAL, at least #17192 doesn't say it.

[maflcko]

You can restart your node if this fails, but I don't think there is a need to crash the node.

[prusnak]

Can we decide what to do with this one?

Should we just drop the CHECK_NONFATAL(g_scan_progress == 0); line?

I really want this to get into 0.21 @luke-jr @promag

[promag]

Should we just drop the CHECK_NONFATAL(g_scan_progress == 0); line?

@prusnak I tend to like these invariants, it makes easier to understand the code. I think @luke-jr concern could be addressed by:

const int scan_progress = g_scan_progress.exchange(0);
CHECK_NONFATAL(scan_progress == 0);

I think @jonasschnelli, @MarcoFalke, @laanwj should take a look too for 0.21.

[jonatack]

Concept ACK. In the absence of test coverage, I agree this should check and fail somewhat loudly if g_scan_progress isn't 0 here.

[achow101]

Code review ACK 8c4129b

[laanwj]

Sorry for bumping the milestone here again. It seems almost ready for merge (could use one more ACK) but we don't want to rush a backport into 0.21.1.

[laanwj]

This came up in the IRC meeting. Adding this to the 22.0 milestone so it can hopefully be included there.

[fanquake]

Backported in #22580.