MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.88k stars 847 forks source link

TransactionBuilder.Verify fails but transaction is successful on the blockchain #339

Closed bazooka70 closed 6 years ago

bazooka70 commented 6 years ago

What could cause TransactionBuilder.Verify to return false? This transaction is successfully signed, and I'm able to broadcast it to the blockchain, but for some reason it always returns "false". e.g.

var builder = new TransactionBuilder();
            var tx = builder
                .AddCoins(coinsToSpend)
                .AddKeys(signingKeys.ToArray())
                .Send(addressToSend, amountToSend)
                .SendFees("0.00001008")
                .SetChange(changeScriptPubKey)                
                .BuildTransaction(true);

Console.WriteLine(builder.Verify(tx)); // always false

The result transaction looks like:

{
  "hash": "b7090c8a07059d1e469e95b73deb42daaa2e1d7e4c9aa4d04ad16de13351a5c5",
  "ver": 1,
  "vin_sz": 1,
  "vout_sz": 2,
  "lock_time": 0,
  "size": 223,
  "in": [
    {
      "prev_out": {
        "hash": "53911e43ec41e5a3b74aed43c3f1368514ac366ac6fc7b4b991853012058adac",
        "n": 1
      },
      "scriptSig": "30440220602d34b8bc0547732753f9251bb8dcbe9dbf039e3af8e88017472d6684e2287b022016c79e734cdad383cf504a079068b1a111bd6715fda164b7bb6e4a1c0bc1da3101 02312d1962a3c7626228c555684971f84e0008fd364e6646303885099be9b69af1"
    }
  ],
  "out": [
    {
      "value": "0.29998992",
      "scriptPubKey": "OP_DUP OP_HASH160 33ab50867662afb080281e0bdb389751bb2c8a08 OP_EQUALVERIFY OP_CHECKSIG"
    },
    {
      "value": "0.20000000",
      "scriptPubKey": "OP_HASH160 f46ba62dc8063b6e798076609d69b1e6803be6e9 OP_EQUAL"
    }
  ]
}

The amounts and fee seems OK, and the fact that the transaction is accepted on the blockchain just drives me crazy. Please assist. I am currently using a single Testnet address for my transactions (including the change address)

Here are all the successful transactions I made:

https://live.blockcypher.com/btc-testnet/address/mkE9yyrgdttSRY7UYQLVUHVY9g7Saqh2Qv/

knocte commented 6 years ago

@NicolasDorier Verify() should probably return void (instead of bool), and throw exceptions for each verification failure.

NicolasDorier commented 6 years ago

@bazooka70 there is probably a good reason which would make it not work on mainnet.

Policy.TransactionPolicyError[] errors =  = null;
builder.Verify(tx, out errors);

check errors then.

bazooka70 commented 6 years ago

@NicolasDorier , Thanks. I got an error: "Fee too low, actual is 0.00001008, policy minimum is 0.00001120". I changed the Fee to 0.00001120 and now it returned True.

I actually used 0.00001008 from Bitcoin Core "estimatesmartfee(6)" when I was testing. so why is the builder complains and where does it gets that Fee number from? Also, Can we eliminate this internal check?

NicolasDorier commented 6 years ago

Better not, it just catched a bug from you... You should use SendEstimatedFee not SendFee, estimatesmartfee sends back a FeeRate (BTC/KB), not a fee.

bazooka70 commented 6 years ago

@NicolasDorier , I think I'm lost... :(


           FeeRate feeRate = new FeeRate(new Money(0.00001264m, MoneyUnit.BTC));

            var builder = new TransactionBuilder();
            var tx = builder
                .AddCoins(coinsToSpend)
                .AddKeys(signingKeys.ToArray())
                .Send(addressToSend, amountToSend)
                .SetChange(changeScriptPubKey)
                .SendEstimatedFees(feeRate)                                                
                .BuildTransaction(true);

Verify Returns error

Fee too low, actual is 0.00000283, policy minimum is 0.00001120

NicolasDorier commented 6 years ago
FeeRate feeRate = new FeeRate(new Money(0.00001264m, MoneyUnit.BTC), 1000);

Bitcoin core is sending BTC per KB.

NicolasDorier commented 6 years ago

does 0.00001264m come direclty from bitcoin core ?

NicolasDorier commented 6 years ago

Ok so I checked, it turned out that bitcoin core dropped down the minTxRelayFee from 5000 satoshi to 1000 satoshi per KB for the network, thus your fee estimation was below what NBitcoin consider safe.

You code is fine, try version 4.0.0.56 which should fix your issue. There is a way to disable this check TransactionBuilder.Policy.MinRelayTx (or something like that), but please do not remove it, it might catch bug in your code.

bazooka70 commented 6 years ago

@NicolasDorier , Thanks. estimatesmartfee(6) returns 0.00004990 on MainNet; estimatesmartfee(6) returns 0.00001016 on TestNet

So the formula I need to use is (Pseudo): FeeRate feeRate = new FeeRate(new Money(estimatesmartfee(6), MoneyUnit.BTC), 1000); ?

Does the 1000 (size) represent the KB?

NicolasDorier commented 6 years ago

Yes, @bazooka70 just update the package your code as before should work. By default the second parameter is 1000 so I don't think it will change anything.

bazooka70 commented 6 years ago
Policy.TransactionPolicyError[] errors =null;
builder.Verify(tx, out errors);
if (errors != null)
{
    foreach (var e in errors)
    {                             
        Console.WriteLine("builder error-> " + e.ToString());
    } 
}

@NicolasDorier I think it is important to add something like e.ErrorCode (enumerator maybe?) for each error in the errors array so we can know what the error refers to, and return our own error message based on the ErrorCode. What do you think? Please don't ask me to make a PR... :-P

NicolasDorier commented 6 years ago

Well you can already do that, all errors are strongly typed so you can use the type of the error to print the right message.

bazooka70 commented 6 years ago

I'm not sure what you mean by strongly typed since all errors in the collection are of type TransactionPolicyError.

NicolasDorier commented 6 years ago

This is only the base type. Look in NBitcoin.Policy for all different type of errors.

bazooka70 commented 6 years ago

Got it! thanks.