slpdb incorrectly index slp address on regtest

[SuperCipher]

When I use slpdb with regtest node the address that get index into mongodb is still a slptest.

possible patch implementation
ActorForth@e79f0ee
ActorForth@805a03e

If you are ok with it I can create a pull request.

[jcramer]

It seems using slptest formatted address should be fine to use for regtest scenarios. You can simply convert it as needed using Bchaddrjs-slp npm package.

…

Sent from my iPhone

On Feb 25, 2021, at 4:07 AM, Napat ***@***.***> wrote:  When I use slpdb with regtest node the address that get index into mongodb is still a slptest. possible patch implementation ***@***.*** ***@***.*** If you are ok with it I can create a pull request. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[SuperCipher]

Thank you for replying @jcramer

It seems using slptest formatted address should be fine to use for regtest scenarios.

Correct me if I'm wrong. You want to keep SLPDB to continue indexing with slptest format even though it's connected to regtest Node?

[jcramer]

Correct. There isn’t anything wrong about it as is. If for some reason you need regtest format then it’s easy to convert it as needed.

…

On Thu, Feb 25, 2021 at 12:08 PM Napat ***@***.***> wrote: Thank you for replying @jcramer <https://github.com/jcramer> It seems using slptest formatted address should be fine to use for regtest scenarios. Correct me if I'm wrong. You want to keep SLPDB to continue indexing with slptest format even though it's connected to regtest Node? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#82 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHRII7G2MGXL4VDSVZSWATTAZ7Z5ANCNFSM4YGDMXKQ> .

[SuperCipher]

Correct. There isn’t anything wrong about it as is. If for some reason
you need regtest format then it’s easy to convert it as needed.

You are correct in the sense that there is no real financial jeopardize.
But there might be an issue in the development namely.

Client or lib that connect to SLPDB has to know in advance that the SLPDB they are query from is running on regtest instead of having an address format to ensure they are operating on the correct data. regtest should be separate from testnet since it operates very differently and should handle differently to avoid possible confusion that might lead to bugs.

Realworld scenario that may cause the problem.
1.
You deploy regtest node and SLPDB for your team to rapidly testing. Your team library depend on SLPDB and point to localhost:12300 after running library code instead of getting slpreg as expect it's got slptest. Now developers have to handle all possible places where the SLPDB query may return slpaddress. Which is no small feat considering how complicated a data SLPDB can return. Instead of just indexing the right data into DB as it should be. Putting immense workload to library creator or client that connects to it.
2.
You deploy regtest node and SLPDB for public use and some users try to query data and got slptest instead of slpreg as expect from regtest node. That can cause confusion.
3.
If a library already handling slpreg correctly then instead of just using it now we have to create some kind of middleware to handle address translation.
Single Source of Truth (SSOT) is a best practice that we should follow

https://www.wikiwand.com/en/Single_source_of_truth

I know you incredibly busy since you maintain multiple repos so it's natural that you might not have the time to review this seemingly trivial change. In that case, you can add me as a maintainer so I can help you improve the repo without sacrificing your time.

Sorry for the long wind
Thank you @jcramer

[jcramer]

Now developers have to handle all possible places where the SLPDB query may return slpaddress

Ok. Although it seems trivial to have the client convert slpreg format using a single method or vice versa.

Would you link me to the code you're working on?

[jcramer]

Also there are multiple testnets so how do you suggest we handle that? Should there be a separate prefix for every kind of testnet?

IMO we need to just stick with mainnet and testnet prefixes. Going further opens up a can of worms. People going to start wanting special testnet4 prefixes to distinguish coins held on testnet3.

Seems like a slippery slope.

[SuperCipher]

Would you link me to the code you're working on?

@jcramer
It is experimental I will change the branch name later
master...ActorForth:develop

[nicolaiskye]

Hello @jcramer , @SuperCipher and I work together so maybe I can help clarify our perspective a bit more.

I think since node software itself provides all three (bitcoincash, bchtest, and bchreg) it would make sense that SLPDB would be able to store the appropriate prefix for the network it's running on.

Correct. There isn’t anything wrong about it as is. If for some reason you need regtest format then it’s easy to convert it as needed.

Ok. Although it seems trivial to have the client convert slpreg format using a single method or vice versa.

While perhaps trivial, it makes things more complicated for libraries that talk to SLPDB. Bchaddrjs-slp may already have the functions to convert to and from, but what about other libraries implemented in Python, Rust, etc.? It seem complexity is kept to a minimum if SLPDB simply indexes the addresses for the appropriate network it's on instead of passing responsibility down the line for other software to handle.

Also there are multiple testnets so how do you suggest we handle that? Should there be a separate prefix for every kind of testnet?

IMO we need to just stick with mainnet and testnet prefixes. Going further opens up a can of worms. People going to start wanting special testnet4 prefixes to distinguish coins held on testnet3.

Seems like a slippery slope.

I agree each testnet doesn't need its own prefix, but I do think since nodes themselves work with the 3 prefixes (main, test, reg) that it makes sense to support those three in SLPDB.

One reason we're pushing for this change is because we aim at providing an easy to use 'local environment' for devs to use to try things out without the monetary requirement of mainnet or the instability of some of the various testnets.

Feel free to check out our project here:
https://github.com/actorforth/bch-toolkit

When our team started developing on BCH last year it required a huge upfront investment in learning how everything works together, and developing directly against mainnet (as many seem to do) doesn't seem like an ideal way to onboard new devs. We hope that having an environment that runs: a regtest node, indexer, REST API, SLPDB, and other software that mimics the existing stack on mainnet would be a benefit to the overall community and help more newcomers join in the development on BCH. Personally I already helped get Bitcash (a Python BCH library) onboard with using the local environment, and we've had several people start doing their own local development with bch-toolkit.

We're happy to submit the PR for the changes, but I hope you might see the value in having a local environment like what we're trying to provide. Having complete control over block generation, no need for a testnet faucet, etc .. makes it much nicer to try out edge cases and experiment with new ideas.

[jcramer]

I am fine with making this change. However, what I don't want to do is make significant changes to Bchaddress-slp. It seems that lib already provides the ability to convert addresses to regtest format, so then slpdb should do the appropriate conversation. PR are welcome that follow this approach.