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

fix: added error with clear message when signing a tx without outputs #2110

Conversation

gomezdn
Copy link
Contributor

@gomezdn gomezdn commented Jun 14, 2024

There's this line of code this.outs.map(output => 8 + varSliceSize(output.script)).reduce((a, b) => a + b) that, when the unsigned transaction has no outputs set, this.outs is an empty array and the only error it throws says "Reduce of empty array with no initial value", which makes one dig into the bitcoinjs code and understand what it was trying to do. Now the error message helps to understand what is happening.

@junderw
Copy link
Member

junderw commented Jun 14, 2024

Perhaps adding a default value of 0 is a better solution.

@IOVgomezdn
Copy link

IOVgomezdn commented Jun 14, 2024

Perhaps adding a default value of 0 is a better solution.

Yes that was my first solution, but then I thought, why one would sign a transaction with no outputs? It's probably a mistake, and an error would prevent the user from broadcasting it.

@gomezdn
Copy link
Contributor Author

gomezdn commented Jun 14, 2024

also, if somebody can give me a hint about the ci test that failed it would be appreciated

@junderw
Copy link
Member

junderw commented Jun 14, 2024

also, if somebody can give me a hint about the ci test that failed it would be appreciated

See #2109

@gomezdn
Copy link
Contributor Author

gomezdn commented Jun 15, 2024

also, if somebody can give me a hint about the ci test that failed it would be appreciated

See #2109

so? what do you suggest me to do? Also what do you think about the fix? adding a default value is better or the error?

@junderw
Copy link
Member

junderw commented Jun 15, 2024

so?

This is rude. Since you seem to be based in a country where English is not your native language, I take no offence from it. But for future reference, "so?" as a response to anything is pretty rude.

what do you suggest me to do?

  1. Read the PR I linked.
  2. Notice the discussion about the same issue (audit CI fails)
  3. Notice the commit that was added by the PR author after the discussion (1a868e8)

Or just wait until the other PR is merged and then rebase on master. Either one is fine.

Also what do you think about the fix? adding a default value is better or the error?

Either one is fine. I just wanted to clarify the reasoning behind why you chose one solution over the other.

@junderw
Copy link
Member

junderw commented Jun 15, 2024

I merged #2113 to master.

@junderw junderw force-pushed the fix/improve-error-message-when-signing-tx-with-no-outputs branch from 07a7262 to 4802803 Compare June 15, 2024 04:27
@junderw junderw merged commit c6105c4 into bitcoinjs:master Jun 15, 2024
12 checks passed
@gomezdn
Copy link
Contributor Author

gomezdn commented Jun 15, 2024

so?

This is rude. Since you seem to be based in a country where English is not your native language, I take no offence from it. But for future reference, "so?" as a response to anything is pretty rude.

what do you suggest me to do?

  1. Read the PR I linked.
  2. Notice the discussion about the same issue (audit CI fails)
  3. Notice the commit that was added by the PR author after the discussion (1a868e8)

Or just wait until the other PR is merged and then rebase on master. Either one is fine.

Also what do you think about the fix? adding a default value is better or the error?

Either one is fine. I just wanted to clarify the reasoning behind why you chose one solution over the other.

so sorry! By no means I wanted to be rude. Thanks for clarifying it. Yes I'm not an English native speaker, will have it in mind from now on.

I asked because I didn't want to interfere beyond the purpose of my fix. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants