Does processedAliases evaluates to false if there is no processed alias (empty array) ? That would be a bug

I'm not sure that's the expected behavior. If your indexer restarted and is connecting older blocks that doesn't mean there is any error on the alias server

You probably want to bail early if unprocessedAliasTxs.length == 0

That's a very bad way to achieve the feature, as it requires you to fetch the whole DB for each alias.
You should rather loop through your unprocessed alias and see if you get a match in the db for each one. It is very likely that there are less alias to register than there are alias already registered. It's also more likely to not saturate the db cache.

Same here, just bail out if there is nothing to do

return false

That's wrong. If there is no alias in the block you should still update the server state to that block.

So this should really be 2 functions:

1. A function to register the new aliases, that simply returns true if it succeeds (even if there is nothing to do) and false in case of a failue
2. A function to update the server state, only called if the above returned true.

That's unrelated to this diff

No -- a unit test has been added for this in this diff (see change to dbTests.js)

It evaluates to []

In this case, processedBlockheight will be 0

We probably don't need this check. I'm not sure how this situation would happen.

good point. this will take a bit of refactoring.

That's what's happening here (since we don't bail above if no aliases, at least, not with the current logic).

confirmedUnprocessedTxs are any confirmed txs at the registration address not yet seen by the server, alias or not.

So at this point -- "even if we found no aliases, if we did have previously unseen confirmed txs, update the server state to reflect this"

The return on line 196 handles the case of "we found aliases, but could not add them to the db for some reason". In this case, we do not want to update the server state, because the app still needs to process those aliases from the last server state.

This was added to make sure startup case (no valid aliases in db) would not crash the app

I don't think this makes sense and this is a very brittle strategy.
I tried to look a bit at how mongo deals with insert errors but the doc is more or less providing a stupid simple working example only, which is not very helpful;

So here is the issue:

1. For each aliases: a. You check if it is in the db or in the list of aliases to be registered b. If not already there, you add it to the list of aliases to be registered
2. You insert the list of aliases to be registered to the db.

This is brittle because it assumes that the state doesn't change between 1. and 2. AFAICT that's the case due to the reentrency check but that will become less obvious after 0-conf tx is available. Also I have zero idea what happens if 2. fails and how to recover from that.

IMO this is a better solution:

1. For each aliases, attempt to insert it into the db

That's it. Because the aliases are ordered, you will insert the correct one and duplicates will error out. You don't even need the local list of aliases to be registered to remove the duplicates from the supplied list. If needed, you can even return the list of success/failures for later use or tests.

TIL that's super confusing. So in js if([]) is true, but if([] == false) is also true because then it converts to a string, and an empty string is (as opposed to empty array or object) false.

For clarity I suggest you use if (processedAliases === false) here.

I mean chronik could reindex while the alias server keeps running. In this case processedBlockheight will not be 0 !

I think the check is good to avoid running the whole logic for each past block, the only question being whether it should return true of false since it's not a failure.
Also this raises the question of how do you handle reorgs.

OK but that's not what the test do. It tests that getAliasStringsFromValidAliasTxs returns an empty array if there is no alias. That's great but that's not testing any of the added logic. This is purely a test case for getAliasStringsFromValidAliasTxs that was missed before

The name is no longer representative of what the function do

Layout nit

unconfirmedBlockheight does not belong to config, because it's clearly not a config option.

Also this function should not check for the tx confirmation, this is the job for some other code. You even removed such txs before the call, so you know that already.

Let's keep this function simple: call it registerAliases, order then store each alias to the db, and return the added ones.
The sorting function already exists and should have its unit tests.
This function can be added separately with its own tests.
Then you can wrap around the logic for actually handling the registrations upon block finalization.

You don't need all these extra variables, just use if (await ddOneAliasToDb(db, thisAliasTx)) { and save 1 byte of RAM.

1. Why do you need to remove the _id key ? Is that a problem ?
2. If it is, why do you need all the data and not only the alias (which is the unique key) ?

Use a constant to avoid the magic number

That's useless, you want to update the server state if there was no issue, even if there was no new alias

And in fact there is no loop to exit

All of this (+the addOneAliasToDb function itself) could be its own diff

that's not a config. It's a db error, it belongs in db.js

Should be >=

Why do you insist on this ? Just do processedBlockheight: tipHeight.

That's still not a loop

Check the equal case as well