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

Coordinator Occasionally Refuses Witnesses #716

Closed
reactive-relations opened this Issue Oct 10, 2018 · 27 comments

Comments

Projects
None yet
4 participants
@reactive-relations

reactive-relations commented Oct 10, 2018

General Description

I started a coinjoin operation. It was partly working, took out some of the jobs from the queue as well.
For hours my coins were not mixed, even though I saw a round of mixing being finished.

System.Net.Http.HttpResponseMessageExtensions.ThrowRequestExceptionFromContentAsync(HttpResponseMessage me) in C:\Users\Adam\WalletWasabi\WalletWasabi\Extensions\HttpResponseMessageExtensions.cs:line 89
at WalletWasabi.WebClients.Wasabi.ChaumianCoinJoin.AliceClient.CreateNewAsync(Network network, InputsRequest request, Uri baseUri, IPEndPoint torSocks5EndPoint) in C:\Users\Adam\WalletWasabi\WalletWasabi\WebClients\Wasabi\ChaumianCoinJoin\AliceClient.cs:line 41
at WalletWasabi.WebClients.Wasabi.ChaumianCoinJoin.AliceClient.CreateNewAsync(Network network, BitcoinAddress changeOutput, Byte[] blindedData, IEnumerable1 inputs, Uri baseUri, IPEndPoint torSocks5EndPoint) in C:\Users\Adam\WalletWasabi\WalletWasabi\WebClients\Wasabi\ChaumianCoinJoin\AliceClient.cs:line 69 at WalletWasabi.Services.CcjClient.TryRegisterCoinsAsync(CcjClientRound inputRegistrableRound) in C:\Users\Adam\WalletWasabi\WalletWasabi\Services\CcjClient.cs:line 485 2018-10-10 05:08:12 ERROR CcjClient: System.Net.Http.HttpRequestException: Bad Request Input is banned from participation for 1381 minutes: 16:..... at System.Net.Http.HttpResponseMessageExtensions.ThrowRequestExceptionFromContentAsync(HttpResponseMessage me) in C:\Users\Adam\WalletWasabi\WalletWasabi\Extensions\HttpResponseMessageExtensions.cs:line 89 at WalletWasabi.WebClients.Wasabi.ChaumianCoinJoin.AliceClient.CreateNewAsync(Network network, InputsRequest request, Uri baseUri, IPEndPoint torSocks5EndPoint) in C:\Users\Adam\WalletWasabi\WalletWasabi\WebClients\Wasabi\ChaumianCoinJoin\AliceClient.cs:line 41 at WalletWasabi.WebClients.Wasabi.ChaumianCoinJoin.AliceClient.CreateNewAsync(Network network, BitcoinAddress changeOutput, Byte[] blindedData, IEnumerable1 inputs, Uri baseUri, IPEndPoint torSocks5EndPoint) in C:\Users\Adam\WalletWasabi\WalletWasabi\WebClients\Wasabi\ChaumianCoinJoin\AliceClient.cs:line 69
at WalletWasabi.Services.CcjClient.TryRegisterCoinsAsync(CcjClientRound inputRegistrableRound) in C:\Users\Adam\WalletWasabi\WalletWasabi\Services\CcjClient.cs:line 485
2018-10-10 05:08:29 ERROR CcjClient: System.Net.Http.HttpRequestException: Bad Request
Input is banned from participation for 1381 minutes: 16:.....
at System.Net.Http.HttpResponseMessageExtensions.ThrowRequestExceptionFromContentAsync(HttpResponseMessage me) in C:\Users\Adam\WalletWasabi\WalletWasabi\Extensions\HttpResponseMessageExtensions.cs:line 89
at WalletWasabi.WebClients.Wasabi.ChaumianCoinJoin.AliceClient.CreateNewAsync(Network network, InputsRequest request, Uri baseUri, IPEndPoint torSocks5EndPoint) in C:\Users\Adam\WalletWasabi\WalletWasabi\WebClients\Wasabi\ChaumianCoinJoin\AliceClient.cs:line 41
at WalletWasabi.WebClients.Wasabi.ChaumianCoinJoin.AliceClient.CreateNewAsync(Network network, BitcoinAddress changeOutput, Byte[] blindedData, IEnumerable1 inputs, Uri baseUri, IPEndPoint torSocks5EndPoint) in C:\Users\Adam\WalletWasabi\WalletWasabi\WebClients\Wasabi\ChaumianCoinJoin\AliceClient.cs:line 69 at WalletWasabi.Services.CcjClient.TryRegisterCoinsAsync(CcjClientRound inputRegistrableRound) in C:\Users\Adam\WalletWasabi\WalletWasabi\Services\CcjClient.cs:line 485 2018-10-10 05:08:48 ERROR CcjClient: System.Net.Http.HttpRequestException: Bad Request Input is banned from participation for 1380 minutes: 16:..... at System.Net.Http.HttpResponseMessageExtensions.ThrowRequestExceptionFromContentAsync(HttpResponseMessage me) in C:\Users\Adam\WalletWasabi\WalletWasabi\Extensions\HttpResponseMessageExtensions.cs:line 89 at WalletWasabi.WebClients.Wasabi.ChaumianCoinJoin.AliceClient.CreateNewAsync(Network network, InputsRequest request, Uri baseUri, IPEndPoint torSocks5EndPoint) in C:\Users\Adam\WalletWasabi\WalletWasabi\WebClients\Wasabi\ChaumianCoinJoin\AliceClient.cs:line 41 at WalletWasabi.WebClients.Wasabi.ChaumianCoinJoin.AliceClient.CreateNewAsync(Network network, BitcoinAddress changeOutput, Byte[] blindedData, IEnumerable1 inputs, Uri baseUri, IPEndPoint torSocks5EndPoint) in C:\Users\Adam\WalletWasabi\WalletWasabi\WebClients\Wasabi\ChaumianCoinJoin\AliceClient.cs:line 69
at WalletWasabi.Services.CcjClient.TryRegisterCoinsAsync(CcjClientRound inputRegistrableRound) in C:\Users\Adam\WalletWasabi\WalletWasabi\Services\CcjClient.cs:line 485

Process is terminating due to StackOverflowException.
2018-10-10 06:05:52 DEBUG StatusLine: System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: index
at System.Collections.Generic.List`1.get_Item(Int32 index)
at WalletWasabi.Http.Models.StatusLine.CreateNew(String statusLineString) in C:\Users\Adam\WalletWasabi\WalletWasabi\H

Screenshots

If applicable, add screenshots to help explain your problem.

Operating System

Windows 10

@nopara73

This comment has been minimized.

Collaborator

nopara73 commented Oct 10, 2018

No user signed for some reason.

My theory is the Tor instance or the Internet of the coordinator went offline for a few seconds in signing phase at 02:08, UTC. Thus, the coordinator banned who did not sign the transaction, which was everyone.

This is quite unfortunate and there wasn't any example of this happening before.

I manually removed all the bans today.

@nopara73 nopara73 closed this Oct 10, 2018

@nopara73

This comment has been minimized.

Collaborator

nopara73 commented Oct 10, 2018

Actually we could even make sure that's what happened if we look at client side logs. Which I was stupid and clear my own logs this morning and. Could you show what are the logs during when the round was gonna execute?

From somewhere around Confirmed connection. Acquired roundHash line.

@nopara73 nopara73 reopened this Oct 10, 2018

@nopara73 nopara73 changed the title from : Bad Request Input is banned from participation for 1381 minutes to Coordinator Occasionally Refuses Witnesses Oct 10, 2018

@nopara73 nopara73 self-assigned this Oct 10, 2018

@nopara73 nopara73 added the debug label Oct 10, 2018

@nopara73

This comment has been minimized.

Collaborator

nopara73 commented Oct 10, 2018

I've acquired the logs of someone else and it turns out the coordinator refused the signatures of everyone. I don't why. This should be further investigated.

@nopara73

This comment has been minimized.

Collaborator

nopara73 commented Oct 28, 2018

Just happened again. The bad request is thrown here:

// Verify witness.
var cjCopy = Transaction.Parse(round.UnsignedCoinJoin.ToHex(), Network);
cjCopy.Inputs[index].WitScript = witness;
TxOut output = alice.Inputs.Single(x => x.OutPoint == cjCopy.Inputs[index].PrevOut).Output;
if (!Script.VerifyScript(output.ScriptPubKey, cjCopy, index, output.Value, ScriptVerify.Standard, SigHash.All))
{
return BadRequest($"Invalid witness is provided.");
}

@lontivero @NicolasDorier Any idea why would it fail here once in a while? Am I using Script.VerifyScript wrong?

@NicolasDorier

This comment has been minimized.

NicolasDorier commented Oct 28, 2018

It should not be this, but I advise you to use the followingCode:

cjCopy.Inputs.AsIndexedInputs().Skip(index).VerifyScript(new Coin(outpoint, output), out var errors);

It is more readable, and you will get errors.

@nopara73

This comment has been minimized.

Collaborator

nopara73 commented Oct 28, 2018

@NicolasDorier Thanks. I refactored it according to your suggestion. Tests pass locally.

// Verify witness.
// 1. Copy UnsignedCoinJoin.
Transaction cjCopy = Transaction.Parse(round.UnsignedCoinJoin.ToHex(), Network);
// 2. Sign the copy.
cjCopy.Inputs[index].WitScript = witness;
// 3. Convert the current input to IndexedTxIn.
IndexedTxIn currentIndexedInput = cjCopy.Inputs.AsIndexedInputs().Skip(index).First();
// 4. Find the corresponding registered input and convert it to Coin.
(OutPoint OutPoint, TxOut Output) registeredInput = alice.Inputs.Single(x => x.OutPoint == cjCopy.Inputs[index].PrevOut);
Coin registeredCoin = new Coin(registeredInput.OutPoint, registeredInput.Output);
// 5. Verify if currentIndexedInput is correctly signed, if not, return the specific error.
if (!currentIndexedInput.VerifyScript(registeredCoin, out ScriptError error))
{
return BadRequest($"Invalid witness is provided. ScriptError: {error}.");
}

The issue may have been fixed by this refactoring. I'll close this issue, because we cannot reproduce it. (This happens like once or twice a month randomly.) I'll reopen this issue if it ever happens again, luckily we now will get an accurate error message if this will happen.

@nopara73 nopara73 closed this Oct 28, 2018

@NicolasDorier

This comment has been minimized.

NicolasDorier commented Oct 28, 2018

@nopara73

(OutPoint OutPoint, TxOut Output) registeredInput = alice.Inputs.Single(x => x.OutPoint == cjCopy.Inputs[index].PrevOut); 
 Coin registeredCoin = new Coin(registeredInput.OutPoint, registeredInput.Output); 

You can simplify

Coin registeredCoin = alice.Inputs.Where(x => x.OutPoint == cjCopy.Inputs[index].PrevOut).Select(x => new Coin(x.OutPoint, x.Output)).Single(); 
@NicolasDorier

This comment has been minimized.

NicolasDorier commented Oct 28, 2018

You can probably refactor also alice.Inputs to just be of Coin type which is strictly the same as your record. So you don't need the Select.

nopara73 added a commit that referenced this issue Oct 28, 2018

@nopara73

This comment has been minimized.

Collaborator

nopara73 commented Oct 28, 2018

@NicolasDorier Yes, I can and did refactor, works perfectly, thank you!

// Verify witness.
// 1. Copy UnsignedCoinJoin.
Transaction cjCopy = Transaction.Parse(round.UnsignedCoinJoin.ToHex(), Network);
// 2. Sign the copy.
cjCopy.Inputs[index].WitScript = witness;
// 3. Convert the current input to IndexedTxIn.
IndexedTxIn currentIndexedInput = cjCopy.Inputs.AsIndexedInputs().Skip(index).First();
// 4. Find the corresponding registered input.
Coin registeredCoin = alice.Inputs.Single(x => x.Outpoint == cjCopy.Inputs[index].PrevOut);
// 5. Verify if currentIndexedInput is correctly signed, if not, return the specific error.
if (!currentIndexedInput.VerifyScript(registeredCoin, out ScriptError error))
{
return BadRequest($"Invalid witness is provided. ScriptError: {error}.");
}

@NicolasDorier

This comment has been minimized.

NicolasDorier commented Oct 28, 2018

you want to probably log witness and scriptPubkey for debugging

@nopara73

This comment has been minimized.

Collaborator

nopara73 commented Oct 28, 2018

@NicolasDorier I noticed you are using SigHash.Undefined by default, but I was previously using SigHash.All. I want the user to sign all outputs. Won't this change lead to vulnerability? For example the user sends me a witness that doesn't sign all the outputs, thus I won't ban the user and it keeps DoS-ing the server like this forever?

Or SigHash.Undefined should be just fine?

@NicolasDorier

This comment has been minimized.

NicolasDorier commented Oct 28, 2018

Why do you care if he uses SigHash.All? You just want to be sure his signature is valid right?

@nopara73

This comment has been minimized.

Collaborator

nopara73 commented Oct 28, 2018

Indeed, there is no DoS issue here. If a valid signature is provided then the round can go on.
There is still the privacy issue with some SigHash-es, like SigHash.Single though, so enforcing SigHash.All is beneficial from that point of view. However that could only happen if someone would use a software, other than Wasabi.

However if this random nasty bug comes from specifying SigHash.All it may be a better compromise to allow any SigHash to be used until the underlying bug is explored. We'll see in a month or so if the issue still persist or not.

@NicolasDorier

This comment has been minimized.

NicolasDorier commented Oct 28, 2018

Well you can still check the witness, you can trivially parse the signature and check the flag before at worst.

@lontivero

This comment has been minimized.

Contributor

lontivero commented Oct 29, 2018

I am not sure the changes can help to fix this problem. IMO we should log the raw output, cjCopy and index before throwing the exception. The idea is that next time this happens we can debug it to know why the script verification fails, then we can fix it.

As a side note, we could consider using the libconsensus library instead of the NBitcoin script verification.

@nopara73

This comment has been minimized.

Collaborator

nopara73 commented Oct 29, 2018

Yeah, if it fails again let's log cjcopy. I wouldn't do that now, because it can be huge and there might be a DoS issue, someone by submitting many wrongsig request or something. It feels like logging something huge can be a bit of an issue. But if it fails again, I guess we have no choice.

As a side note, we could consider using the libconsensus library instead of the NBitcoin script verification.

Not sure if it's a server side issue. Would be good if it is. Let's wait and see.

@NicolasDorier

This comment has been minimized.

NicolasDorier commented Oct 30, 2018

@lontivero I advise against using libconsensus as I think the bug does not come from this. libconsensus is a pain to work cross plateform and on multi arch. (you still have not solved Mac and Linux builds :p)

@lontivero

This comment has been minimized.

Contributor

lontivero commented Oct 30, 2018

Yes, i know. However, in this case we don't need cross platform support.

Anyway, we don't know the root cause yet but i think it could be in the server side.

@nopara73

This comment has been minimized.

Collaborator

nopara73 commented Nov 1, 2018

This just happened again. Now, with improved logging do we know what this means, how can this happen? Or we should improve the logs further? Invalid witness is provided. ScriptError: NullFail.

For convenience: NullFail is the error here: currentIndexedInput.VerifyScript(registeredCoin, out ScriptError error.

@lontivero

This comment has been minimized.

Contributor

lontivero commented Nov 2, 2018

We need to improve the logs further because that log entry is not enough. Basically the problem seems to be in NBitcoin's ScriptEvaluationContext.cs

bool fSuccess = CheckSig(vchSig, vchPubKey, scriptCode, checker, hashversion);
if(!fSuccess && (ScriptVerify & ScriptVerify.NullFail) != 0 && vchSig.Length != 0)
	return SetError(ScriptError.NullFail);

So, it is the CheckSig that returns false and that can happens for many many reasons.

Update: In other words, the problem is the signature is not valid so, we still know nothing.

@nopara73 nopara73 reopened this Nov 2, 2018

@NicolasDorier

This comment has been minimized.

NicolasDorier commented Nov 2, 2018

I think you should move the search as to why the signature is not valid.

This signature come from a participant? Do you have the source code where participant sign?

@lontivero

This comment has been minimized.

Contributor

lontivero commented Nov 2, 2018

Yes @NicolasDorier the signatures come from participants and the code where they sign the transaction is in the link I shared above.

I've acquired the logs of someone else and it turns out the coordinator refused the signatures of everyone.

This is the weird part, under what circumstance could more than one participant send wrong witnesses to the server?

nopara73 added a commit that referenced this issue Nov 2, 2018

@lontivero

This comment has been minimized.

Contributor

lontivero commented Nov 2, 2018

Just an idea (theory):

  1. User register his output
  2. Coordinator detects fees can be optimized and modify the participants' output amount
  3. At signing time, participants receive an unsigned transaction with modify outputs
  4. Participants sign the tx with their submitted outputs
  5. Given segwit transaction hash includes the outputs amount, they sign a different transaction
  6. Coordinator rejects the witness
@nopara73

This comment has been minimized.

Collaborator

nopara73 commented Nov 2, 2018

It is NBitcoin TransactionBuilder issue at signing.

This doesn't work:

var builder = Network.Main.CreateTransactionBuilder();
var signedCoinJoin = builder
    .ContinueToBuild(tx)
    .AddKeys(keys.ToArray())
    .AddCoins(registeredCoin)
    .BuildTransaction(true);

This works:

var signedCoinJoin = tx.Clone();
signedCoinJoin.Sign(keys.ToArray(), registeredCoin);

We'll fix in the code, but TransactionBuilder further investigation is needed.

@nopara73 nopara73 closed this in e951d65 Nov 2, 2018

@NicolasDorier

This comment has been minimized.

NicolasDorier commented Nov 3, 2018

ContinueToBuild is normally done if you want to add .Send stuff.
You should do

var builder = Network.Main.CreateTransactionBuilder();
var signedTx = builder
    .AddKeys(keys.ToArray())
    .AddCoins(registeredCoin)
    .SignTransaction(tx);
@lontivero

This comment has been minimized.

Contributor

lontivero commented Nov 3, 2018

Yes, confirmed The code used for testing signs the tx as expected without NullFail using SignTransactionInPlace.

Thx, @NicolasDorier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment