fix: nft holdings endpoint doesn't return all NFTs

[usagi32]

addresses #1843

The problem was v1/tokens/nft/holdings endpoint with include_unacnhored set is querying the nft_custody_unanchored table which only contains records which were once part of unanchored microblocks and not of normal anchored blocks. Since conceptually, when include_unacnhored is set, the result should not be exclusive even slightly. On the contrary, the response should be the superset of the response when include_unacnhored = false. It should show all records in addition to all the anchored records.

This could have been approached in many different ways. One solution is to simply query both nft_custody_unanchored and nft_custody tables when calling this endpoint and keeping one unique copy of each record in a set. Another way is to update the nft_custody_unanchored table both times to act as a superset of records. I went with the second one.

In my opinion, both of them are far from ideal. With first, will all have to access the database multiple times and perform some check for uniqueness. This will increase response time. With second one, this can be avoided but will introduce redundancy that too for temporary data that should not be persisted.

I was thinking about this before too as it was odd to me but after this issue a realization emerges that there should be some changes in the way non-final data is handled. Blockchain-api should not permanently persist temporary data like microblocks and mempool related data. As this data is bound to be changed or become non-existent due to its transient nature. This data creates unnecessary redundancy, storage and performance problems over long-time.

Maybe something like Temporary Tables/Temp Tables in postgres, which can be periodically merged into permanent ones as the data finalizes, is the answer.

Maybe some tables can be restructured like for eg. both the nft_custody tables can be combined into one with either corresponding anchored or unanchored columns set to null and as non-final data materializes or vice-versa the finality of those columns can be inverted.

@zone117x

[CLAassistant]

All committers have signed the CLA.

[zone117x]

@CharlieC3 any idea why the PR is missing the "approve & run" CI option that typically shows up in PRs from external repos?

[CharlieC3]

@zone117x My guess is these PRs were created before this setting below was enabled:

Then when you added the blocked label in the other PR, it triggered a workflow, which caused it to ask for approval before running. If you added a label to this PR, it might ask for approval here too.

[zone117x]

@CharlieC3 nice that works, thanks!!

[zone117x]

@usagi32 thanks for the contribution! Can you please add a test to show that this change fixes the bug?

[rafaelcr]

@usagi32 since microblocks no longer exist on Stacks, it might be easier to change the endpoint to ignore the unanchored param completely so it always checks the nft_custody table. Perhaps we could even remove the nft_custody_unanchored table altogether.

[rafaelcr]

Removed this table in #2190, we can close this PR.

Thanks for the nudge to get this issue fixed @usagi32 !