The 0.5.3 release has extra validation of recipient account numbers. While investigating this, I was able to prove that at least one of the reported incorrect transactions, so far unexplained, was due to an user error: User account I was sending: 12956190138975700589
Erroneous account: 434692873790144579
Transaction ID: 4799337629054063359
Amount: 14'699
Account number 12956190138975700589 was incorrectly typed with an extra 1 at the end: 129561901389757005891. This is not a valid account number (it exceeds 2^64) and so resulted in overflow and was interpreted as 434692873790144579. I have added code that checks and prevents such overflows, so from now on this would return an "invalid recipient" error message. It is important to note that typo's that result in a different but valid account number will still not be caught, but in those cases it would be more obvious to the user that he has made a typo. So if the user enters the account as 12956190138975700588 for example, it will be accepted because this is a valid account number. Most importantly, there is no evidence in the above case of a random, memory corruption type of bug, as some have feared. Adding checksums as a way to prevent user errors is a different issue, but there has been no memory corruption at play here. |
|
|
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Released 0.5.3: http://download.nxtcrypto.org/nxt-client-0.5.3.zipsha256: 23fc36fba166e00299003407169a26515e6d67c8094b5a06f9c795cc62ca83a7 Change log: Fixed blockchain rescanning. Clear unconfirmed transactions on rescan. Better recipient account number validation. Do not accepts account numbers that overflow or negative account numbers. Should prevent at lest some cases of transactions being sent to wrong recipients due to user error. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iQIcBAEBAgAGBQJSzTHXAAoJEFOhyXc7+e2ASMMQAMcNPIImqCA1p27zoS2hRquk IPNEOX+19YEeN7Gp5vcYzFC/5NUQONoed1tO29HkcQL+/UjJPrGfYNUKEA5xsUQA Z0NGYmXLTMqKIdgisG8xC8ZDki8I60II82BUOksgxZwtdpjyfajN+jLNUpYjs1HL 4lEEngZ7YfcWt2kxHDF5iIjnn3P0XbvfDasSDMH1OL77bXUfBbdHhSgJzSwL2rmw 8i96wmJ8qa+hLi72n6vazketQxqG0y8oLzzuQJC4nkGRuwaVdg/sAdckpb8PTE/3 mYMuLxvDh/4kMXj5XQHY55B/+UnaRBtjl0MSrDPjqAwesqv7ll50Le/DE1oeQvj2 ZnyZaFI96A48fEHRxcBrmEhTQ6LIvU/cQ6ClYnhqN2mi38f7TvttGjlqUQAy/F6I 67fnsBcIhn8u0vdOMZBcX5mTSczBnRqPlO3nvkH/SKljEw9cuFF9QUlGTfqiX7AX EMZDp7CMQEvqgSCoDYd9ZbZ+ri7oWVh2y/vSIUAl4RkpsA0eaMMS3FixcJyFZS0D tkM2dnL/+BjIQrsrvrfcD67jNhVaoK+V/o/xTmLV5WmNgmv6CUL0KjfXFX1eqHn8 I5FuoKzcZf2Ld4a9WBohCbDNRErg/qFWqMrd0Tae5C94OheZY7Pa4c0oI0Rhvjld xVDjyDc8xhrtmG5nsOdy =GwA8 -----END PGP SIGNATURE----- |
|
|
|
Ok, so 0.5.2 doesn't fix the exploit yet.
It does re-scan the block-chain, but something in there seems to be causing an exception and ...Done is never reached. (I can see in the logs that there was a second re-scanning started).
Fixed, now testing it. |
|
|
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Because one release per day is not enough, here comes 0.5.2: http://download.nxtcrypto.org/nxt-client-0.5.2.zipsha256: 530951bef607935f80326fcd5c50b888498a9b726c6d2bd7127a77395b4404fc Change log: Added validation of numberOfTransactions and payloadLength to prevent OutOfMemory attacks. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iQIcBAEBAgAGBQJSzFKbAAoJEFOhyXc7+e2A0nQP/RchfPjiz/see23wkSnQT2Ob xBPNv6Na3eIG0t2RNeliohJ2/K8RP//vXLSf8vDcPh896AIXKWfiRBupsCu84fsE dpxkE7gklvKtIehKuhOSuWqdOpdUTlDqdxki7OtsDzB/kY/6iqFVbEEnkvMg+s8i IynogQ+wmfx0YO9jyB4CK2lOtyhg7SttS7k6s7tlXLIr+sFSjM7vGSu+AfPJ/lik iRWOg1pJdXRXCV6dPsowMDKIXP0UGMwc419CHhFjjHiDcmtYb/e5/EMhuhrodMnj Mh0HALE27V0INmsISVMibgJeNzq9C3mUVsBEzyBBAOS9cbL3F4hspwgewEZKuDxN hvL3rIrh0XbZzdiXdT6PL2g1olnvL1EgDcHo9oWcDdJd3Qi0C+KHG1nlI6yDH81G OisByW7fTZBJQ6gykcAzB1Pu3pSDDbF2h3Z7CTH4L2rK65azg2JS61z9d6KdUITI rXlsOyTUoyJpwjHj2CBjwBUiU0lQDu3G5xElNHE8OdCiVKER04QQXX4Q1wEJqPe+ U1H5Q1BPSVaCCQj7tZ+fFLnWMbUaXGzp+lxI0m/n9DdhoDkWnL7ykK9s1OLJW0eb 83A7gYdWcguI4pQR3QTyvsymJx6l2mRmz/Xay3hdemCkGJP7NMYD5RNYcBAB5WI6 e3PKlqOtJepjl10Med6N =N9Ib -----END PGP SIGNATURE----- |
|
|
|
It went crazy so I upgraded to 0.5.1 and after it went crazy the exact same
way as 0.5 previously, the only thing that helped was creation of blockchain.nrs in NXT root directory. Everything worked fine afterwards,
node is up few hours now without any signs of going crazy again.
You can keep blockchain.nrs there if it brings you good luck. But the code is not using it. |
|
|
|
Recent blocks [-2920]  [2014-01-07 17:17:37.372] Generated an incorrect block. Waiting for the next one...
[2014-01-07 17:17:38.228] Re-scanning blockchain...
 Did you try reloading the page in the browser, or you had to restart java process to fix it? No other errors in the log (except the usual jetty warning exceptions)? Jean-Luc, like to report, had it again - did a refresh and waited a bit longer this time. java console displays [2014-01-07 15:15:08.995] Generated an incorrect block. Waiting for the next one
...
[2014-01-07 15:15:17.713] Generated an incorrect block. Waiting for the next one
...
[2014-01-07 15:15:32.307] Re-scanning blockchain...
[2014-01-07 15:15:51.448] Generated an incorrect block. Waiting for the next one
... it looks like it catched up now, I see: 35168 4747512364439223888 7. Jan. 2014 15:26:13 11 6'998 + 12 1'416 B 2 5716360880337650150 247 % But balance is wrong, the one in web-gui is too high and not the same as the correct one I get when issuing request: http://localhost:7874/nxt?requestType=getBalance&account=xxxThat's good, so we are on the right track. The server has rescanned the blockchain and knows the correct balances, now I need one more fix to have the GUI updated too. |
|
|
|
Recent blocks [-2920] [2014-01-07 17:17:37.372] Generated an incorrect block. Waiting for the next one...
[2014-01-07 17:17:38.228] Re-scanning blockchain...
Did you try reloading the page in the browser, or you had to restart java process to fix it? No other errors in the log (except the usual jetty warning exceptions)? |
|
|
|
U could start reviewing it.
Ah, well, adding one more Jira issue to my list... |
|
|
|
Java will be gone soon.
huh? Java will be gone soon? Could be wrong? Wasn't there talk of going to C? And a new client? Can someone confirm or deny this? Java will not be gone, that's for sure. There will be a new native client, that nexern is working on. And others can also create different clients, including a replacement for the current javascript client. The server will remain in java. Of course, once the full source is released in April nothing prevents someone from rewriting it in another language. |
|
|
|
If asset exchange API is ready, test servers should be put up now so that others can develop clients too. I see no sense in waiting.
Jean-Luc has not reviewed Asset Exchange code yet, AFAIK. Too much work with refactoring, I think. Is the asset related code I have in 0.5.1 ready to be reviewed? Last time I remember you said you are still fixing it, so I haven't touched it. |
|
|
|
The fix is to call awaitTermination on the threadpools before call Block.saveBlocks. This has been fixed since 0.4.9e. Waiting up to 10 s, then shutdownNow. Guess that's not what is killing 80% of my nodes. heh. What exactly do you see happening, blocks.nxt corrupted? The code now is: scheduledThreadPool.shutdown();
try {
scheduledThreadPool.awaitTermination(10, TimeUnit.SECONDS);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
if (! scheduledThreadPool.isTerminated()) {
logMessage("some threads didn't terminate, forcing shutdown");
scheduledThreadPool.shutdownNow();
}
try {
Block.saveBlocks("blocks.nxt", true);
} catch (Exception e) {
logMessage("error saving blocks.nxt");
}
try {
Transaction.saveTransactions("transactions.nxt");
} catch (Exception e) {
logMessage("error saving transactions.nxt");
}
logMessage("Nxt stopped.");
If any thread didn't finish successfully, you will see the "some threads didn't terminate..." message. But even so, they are then terminated forcefully before trying to save blocks.nxt so shouldn't interfere. I know, one could save to a temporary file and then rename. Don't want to get into unix vs windows file delete differences. Besides, the plan is to move away from saving serialized objects to a file eventually. |
|
|
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Released version 0.5.1: http://download.nxtcrypto.org/nxt-client-0.5.1.ziphttp://info.nxtcrypto.org/nxt-client-0.5.1.zipsha256: 4c6ee12c5be7b49e0622afe2b64a0735c1355e58760f525f4a7dc4688882b656 Note the new URL, after this one new releases will be hosted only at download.nxtcrypto.org instead of info.nxtcrypto.org. Please test the http://localhost:7874/update.html tool to download and verify the package. This is mostly a bugfix release. Change log: Possible fix for https://bitcointalk.org/index.php?topic=397183.msg4343616#msg4343616 , needs more testing. Please report any occurencies of negative Recent blocks and balances, and include the log errors if that happens. Fixed blacklisting of hallmarked peers. Accounts with negative balances can't forge. Improved account public key verification. Updated update.html to the newest version and changed download url. Fixed missing transaction id's in My Transactions. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iQIcBAEBAgAGBQJSy9LmAAoJEFOhyXc7+e2AwEMQAOytTyvGlVj8CgwWWm9BM7o8 Y4srx4Ow+zVPmr5KmkxaR9pVAOxGE+1rCl6D34Tm1ZdyEwwdAcwjATqOx4Gu8l45 VxPKYn1QBC1iL9Y1zYOwhnwmeJQMcsqIR4q0y2IGYjBN8st0Tyv6Jdag5ytX22kJ ApUt+Oxvr1o+6DHd82AODnBvkOUlSmACNNhtwIk9NL5kw4JG54Yv6HS1suzNO2kz KFKuRPr1SzNsp1crPt9Rprh94Crb/1sYjDTbTFu/KfJ5hRdiy7crCuMFnIayp+/M s9H8obGofBb0t6cQ0WNGztYDeEY2Gn1FlPaaXs73ZXdjHcZ0fxxjjL0IR4gXmfd/ OQowfRU0S3dmGFeN2l3k7BT3kfUjbcX3tKyxTgM/TWOjTQp4edLZGZ4ujma8hBGC cFBk89s1oxVPIjkjejGsIbdZWy9nV/fvhcyy2HuW3uFDxGEHJfEagR8I4dKGtcv6 EYS4+FkcCKoGNtOGdAUIxxOxqe9pFpoQRisw0VMaZxLLzE8X+v/FzWYEethMMMIY wgREiFdWiD+NvMdAdAltF8hCilwV7gFimEaUn2SXVTmQpN1ZVbdi5ly4/Uc0uQX+ EWkEqb68YuRgRYoYViqVC6JmMl/uWURHKU/7DPKxjIJGDLEjRvmwChs4YEgDUtfV ipNKrIm9Qvp08QojXmrH =ZQ/M -----END PGP SIGNATURE----- |
|
|
|
The fix is to call awaitTermination on the threadpools before call Block.saveBlocks. This has been fixed since 0.4.9e. Waiting up to 10 s, then shutdownNow. |
|
|
|
FWIW, it's kind of annoying to be reviewing outdated code while the developers are working on a much newer copy. I understand why an older snapshot has been released, but it is still annoying to keep finding things that have already been fixed :/. But, I guess it's good that the working code is in a better state than the released code.
Thanks for your reviews, it could be that you find things I haven't thought of yet. In fact you did in a previous post, now I have added extra checks for Transaction.amount and Transaction.fee in a place where it could overflow. The code is not that outdated actually, it is as of Dec 25 when I first got access to it and we were running it until about Jan 01. |
|
|
|
If you need to ensure they are always consistent, you need to lock around all the reads and writes.
I know, I have done that. All direct reads of Account.balance are replaced with getBalance() which is synchronized. Account.balance is now private (doesn't make a difference now when in the same class, but it will be moved out of Nxt.java one day). |
|
|
|
There was a lot of code that looked like:
synchronized (account) {
1. Check balance
2. Do something
3. Adjust balance
}
Is it safe to get rid of synchronized in the quoted code?
It is not, and I haven't. synchronized (account) {
if (account.getUnconfirmedBalance() < amount * 100L) {
doubleSpendingTransaction = true;
} else {
doubleSpendingTransaction = false;
account.addToUnconfirmedBalance(- amount * 100L);
...
|
|
|
|
All of those synchronized(this) can be replaced with AtomicInteger.addAndGet
How about this: synchronized (this) {
this.balance += amount;
this.unconfirmedBalance += amount;
}
Each of them being Atomic by itself will not ensure balance and unconfirmedBalance are both updated atomically, a thread could still see balance incremented but unconfirmedBalance not yet updated. |
|
|
|
Jean-Luc, thank You for response. What do I need to change in web.xml, in order to have some log file, which will keep information of what is going with console process and saved on disk? I saw somewhere in this thread, but it was long time ago. Or it will be enough to you to have the snapshot of the running console window information inside?
You don't need to change anything in web.xml, the logging option there is for peer communications only. Just select and copy the text from the console and paste it into a text file. If you only do a screenshot, it may not capture something that has scrolled up already. |
|
|
|
There does seem to be a lot of excessive locking. For example: Transaction transaction = Transaction.getTransaction(buffer);
synchronized (Nxt.transactions) {
transaction.index = ++transactionCounter;
}
In 0.5.0 this looks like: Transaction transaction = Transaction.getTransaction(buffer);
transaction.index = transactionCounter.incrementAndGet();
I am afraid CfB and BCNext may get mad at me, but 0.5.0 relies heavily on the Concurrent API... |
|
|
|
Since setBalance can throw, where you do the following, balance and unconfirmed balance can get out of sync: generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);
void setBalance(long balance) throws Exception {
this.balance = balance;
for (Peer peer : peers.values()) {
if (peer.accountId == id && peer.adjustedWeight > 0) {
peer.updateWeight();
}
}
Granted, this is unlikely since you are suppressing all exceptions around the network calls. But it might be more robust to calculate balance from unconfirmedbalance or vice-versa. There is no setBalance() in 0.5.0: synchronized long getBalance() {
return balance;
}
synchronized long getUnconfirmedBalance() {
return unconfirmedBalance;
}
void addToBalance(long amount) throws Exception {
synchronized (this) {
this.balance += amount;
}
updatePeerWeights();
}
void addToUnconfirmedBalance(long amount) throws Exception {
synchronized (this) {
this.unconfirmedBalance += amount;
}
updateUserUnconfirmedBalance();
}
void addToBalanceAndUnconfirmedBalance(long amount) throws Exception {
synchronized (this) {
this.balance += amount;
this.unconfirmedBalance += amount;
}
updatePeerWeights();
updateUserUnconfirmedBalance();
}
|
|
|
|
|
Show Posts
