-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Refactor CreateNewBlock transaction selection algorithm #1240
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
Conversation
luke-jr
commented
May 9, 2012
- Transaction fees for children might help the parent transaction get confirmed
- Transaction fees can boost the priority of transactions included
|
So far, I have done some minimal testing by analyzing the output of getmemorypool. |
|
Eligius has found 2 valid blocks (and none invalid) with this code (plus #1246, for safety) so far, at its default settings. I am now testing with a huge weigh toward transaction fees, to increase the likelihood of dependent transactions paying for their parents (and therefore risking any potential child-before-parent problems). |
|
Things I need to address:
|
|
FWIW, 10 total valid blocks found with this (most recent is …30E6C09A4FF45348A0EF0AA1A), zero issues caught by #1246, and zero invalid blocks logged by bitcoind. |
|
Integrated my 5 "things to address". |
|
Eligius has been running this from block 179513 (56 blocks found) and EclipseMC from 180573 (11 blocks), totalling 67 valid blocks with no problems found. (EMC: 180788(1040); Eligius: …9DA7D49DC2539F9D299AF8E5A) |
|
Also passes the unit tests I just wrote on #1246. |
|
This seems to be be excessively slow, and possibly exploitable right now. |
|
Define "this" ? |
|
This pull request. Eligius is getting stuck in some huge dependency trees, it seems. Trying to figure it out, but don't have time just this second. |
… use %12"PRI64d" instead Conflicts: src/walletdb.cpp
- Transaction fees for children might help the parent transaction get confirmed - Transaction fees can boost the priority of transactions included
Conflicts: src/db.cpp src/init.cpp src/main.cpp src/main.h
|
Fixed the major performance hit from complex dependency trees. Still not as fast as the old algorithm, but not absurdly slow either. I'd say the ability to pay for "parent" transactions is worth it. |
|
(Fix verified by about 10 days of testing on Eligius) |
|
I certainly agree to the idea of this patch. I haven't checked the source yet though. |
|
Need to be very careful not to accidentally introduce a potential DoS attack by arranging transactions that require N^2 time or space to figure out fees/priority. |
|
@gavinandresen I think SatoshiDice tested that for me (the fix from ~16 days ago) |
|
Could you write up a gist that explains what formula for priority you're using? And maybe talk about how it will handle transactions from old clients (who calculate priority the old way) ? |
Old clients don't take priority into consideration at all right now AFAIK, nor does this code (or the code it's replacing) completely exclude transactions based on their priority. |
|
NACK, the priority formula would make it easy for somebody with (say) 100,000 BTC to pay tiny fees and get priority over higher-fee-paying transactions. That's a bad incentive, it would encourage people to do dangerous things like keep more funds in their 'hot' wallet or add extra inputs to their transactions (and just have bigger change outputs) to get higher priority. |
|
So, should I take the input value out of FeeScore? |
|
Sadly, in addition to being NAK'd this conflicts heavily with @gavinandresen 's work redoing CreateNewBlock() You should work with @gavinandresen to coordinate changes in that area, before posting another pull request relating to CreateNewBlock() + TX selection/fees. |
…1240) * Reject getblocktemplate request until masternode sync is finished * Pending internal miner until masternode sync is finished
* Do no relay double spends to SPV clients. Relaying double spends to SPV clients can be easily attacked. An attacker could intially send tx1 to their own wallet. Then send a double spend (tx2) to another merchant's SPV wallet. The merchant will never see tx1 but will see and receive tx2, however, tx2 will not get mined since the miners node will see tx2 but not add it to their mempool. Later, tx1 will get mined leaving the merchant with an invalid/conflicting tx2 and no payment will be confirmed. * Only skip sending Respends to peers that have given us a bloom filter which is not empty.
…xRecord updateStatus. 0057548 [Model][Wallet][Performance] TxRecord updateStatus not accepted stake status fix + performance improvements. (furszy) Pull request description: Several improvements on the `updateStatus` method solving the not accepted stake type not set bug, plus performance improvements. * The transaction record status update was not taking into account the negative depth calculation, neither if it was conflicted, for not accepted coin stakes * GetDepth method was called in three different ways. 1) GetDepthAndMempool, 2) IsTrusted, 3) GetBlocksToMaturity. This PR unifies every call into a single call to IsTrusted(&depth, &fConflicted). * Several calls to chainActive->Height unified into a single one in record statusUpdate. ACKs for top commit: random-zebra: utACK 0057548 Mrs-X: utACK PIVX-Project@0057548 Tree-SHA512: 9517b1bd2bb782bd459ef94972db04a0408429e5f38009f178c915ae24e8240f792c30e884f809a91f605fcc2dae9aa6fef3dd8a74d1e77c973014f1b346c683