Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass network as optional parameter #6

Closed
wants to merge 23 commits into from
Closed

Conversation

Czino
Copy link

@Czino Czino commented Jan 16, 2024

For improved testability and enabling me to develop using regtest, I need to be able to pass the network into the verify method.
This is already the case for signing.

@ACken2
Copy link
Owner

ACken2 commented Jan 17, 2024

Thanks for the pull request. Based on the files changed, it seems like the main objective is to enable BIP137 and BIP322 signature verification for regtest addresses. Is that correct?

@Czino
Copy link
Author

Czino commented Jan 18, 2024

Yes, enabling it for regtest and possibly making it compatible with other networks in future such as the liquid network.

@ACken2
Copy link
Owner

ACken2 commented Jan 19, 2024

Is there any reason why the network cannot be inferred from the address given, but have to be exposed directly?

@Czino
Copy link
Author

Czino commented Jan 19, 2024

Is there any reason why the network cannot be inferred from the address given, but have to be exposed directly?

Yes, I can think of 2 reasons:

  • adhering to DRY principles: no need to rebuild the address for every network (adding regtest here would mean to extend the return value of convertPubKeyIntoAddress by regtest)
  • more importantly: if you are running the code in a mainnet environment, one does not necessarily want to accidentally validate testnet addresses

@ACken2 ACken2 self-assigned this Jan 19, 2024
@ACken2
Copy link
Owner

ACken2 commented Jan 19, 2024

Is there any reason why the network cannot be inferred from the address given, but have to be exposed directly?

Yes, I can think of 2 reasons:

  • adhering to DRY principles: no need to rebuild the address for every network (adding regtest here would mean to extend the return value of convertPubKeyIntoAddress by regtest)
  • more importantly: if you are running the code in a mainnet environment, one does not necessarily want to accidentally validate testnet addresses

As far as I know, signature that is valid for a mainnet address is equally valid for its testnet equivalence, and vice versa. By not wanting to "accidentally validate testnet addresses", do you mean you don't want to accidentally pass a testnet address into the verifySignature method when you only want to validate mainnet addresses?

@Czino
Copy link
Author

Czino commented Jan 19, 2024

do you mean you don't want to accidentally pass a testnet address into the verifySignature method when you only want to validate mainnet addresses?

Yes, it's also the standard behaviour of bitcoin core, when an address is provided that is not for the same network. Invalid address is returned.

@ACken2
Copy link
Owner

ACken2 commented Jan 19, 2024

do you mean you don't want to accidentally pass a testnet address into the verifySignature method when you only want to validate mainnet addresses?

Yes, it's also the standard behaviour of bitcoin core, when an address is provided that is not for the same network. Invalid address is returned.

I am still doubtful about this particular use case.

The signature itself is network-neural, and it is valid for all addresses that can be derived based on the corresponding public key. Other libraries such as the widely used bitcoinjs-message also does not impose such limit on the network - the same signature would verify as true for both mainnet and testnet address without additional configuration.

The problem of "accidentally passing in a testnet address" sounds like a user-input validation problem, rather than an issue that must be dealt with in this library.

While this could be a useful feature to restrict signature verification on a specific network, I am doubtful on whether it should be mandatory. In my opinion, it is better to infer the network automatically, but allow the option to restrict to a particular network type (as a bonus feature).

@Czino
Copy link
Author

Czino commented Jan 22, 2024

I changed the code so the network is inferred at the beginning of the verification routine.

Therefore the scope of the PR changed to

  • support regtest addresses
  • refactoring

@Keegan-lee
Copy link

When is this PR getting merged?

@ACken2
Copy link
Owner

ACken2 commented Mar 28, 2024

Sorry, but I currently do not have the time to review this pull request in details. This may get merged in late April or early May.

@ACken2 ACken2 added the enhancement New feature or request label Apr 3, 2024
@ACken2 ACken2 mentioned this pull request May 15, 2024
@ACken2
Copy link
Owner

ACken2 commented May 15, 2024

Closing in favor of pull request #10.

Thank you for your contribution.

@ACken2 ACken2 closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants