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

Cache hashes #70

Closed
wants to merge 1,445 commits into from
Closed

Cache hashes #70

wants to merge 1,445 commits into from

Conversation

NicolasDorier
Copy link

Some notes about the implementation:

  • It calculates the three midstate hashes as soon as a CheckSig segwit operation happens, whathever the SigHash,
  • It is possible that two different threads calculate the midstate hashes of a transaction twice,

Befinits are:

  • hashcashes map access is limited so we don't have too much lock contention
  • Fewer conditional branches in consensus code
  • Simple to review

This commit is only for having a cache that is simple to review and understand. It is probably possible to fix the two first points above, but the code overhead is not worth it when our goal is only to fix the O(n²) issue.

@NicolasDorier NicolasDorier mentioned this pull request Apr 11, 2016
uint256 SignatureHash(const CScript &scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, const CAmount& amount, int sigversion);
uint256 SignatureHash(const CScript &scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, const CAmount& amount, int sigversion, CachedHashes& cache);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combine with previous with default arg of NULL?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait why does it compiles at all ? my implementation is CachedHashes* not CachedHashes& will fix that. oO

@instagibbs
Copy link

What's the primary differences between this pull and #63?

@NicolasDorier
Copy link
Author

The only difference should be that I removed the tests fixes which were cherry picked from #68

@NicolasDorier NicolasDorier force-pushed the cachedhashes branch 4 times, most recently from 53d4fbf to ce793c3 Compare April 12, 2016 14:19
@NicolasDorier
Copy link
Author

added some comments, and check if the scriptPubKey is actually segwit before consulting the cache (https://github.com/sipa/bitcoin/pull/70/files#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR1818)

@NicolasDorier
Copy link
Author

rebased, added description to the PR.

@sipa sipa force-pushed the segwit4 branch 4 times, most recently from 1218bb6 to fb4873e Compare April 17, 2016 23:17
@sipa sipa mentioned this pull request Apr 19, 2016
7 tasks
fanquake and others added 10 commits June 2, 2016 09:16
The plist is generated, lives in builddir.
2692e1b [Doc] Simplify OS X build notes (fanquake)
16698cb PR bitcoin#7772 is not enough to fix the issue with QCompleter, use event filter instead of `connect` (UdjinM6)
Before this, if someone imported a scriptPubKey directly (in hex form) using
importaddress, outputs sending to it would be treated as change, as the
corresponding CTxDestination was not added to the address book.

Fix this by trying to detect scriptPubKeys that are in fact convertible to a
CTxDestination and add them anyway. Add a warning to the RPC help to warn
against importing raw non-standard scripts.
269a440 Add test for dbwrapper iterators with same-prefix keys. (Matt Corallo)
6030625 test: Add more thorough test for dbwrapper iterators (Wladimir J. van der Laan)
84c13e7 chain: Add assertion in case of missing records in index db (Wladimir J. van der Laan)
laanwj and others added 22 commits June 20, 2016 13:41
…for each node

fad1845 [qa] test_framework: Use different rpc_auth_pair for each node (MarcoFalke)
595b22e Stop treating importaddress'ed scripts as change (Pieter Wuille)
1e9aab0 Remove sipa's old revoked key from verify-commits (Peter Todd)
966151e Add README for verify-commits (Peter Todd)
11164ec Remove keys that are no longer used for merging (Peter Todd)
22421fa Remove pointless warning (Peter Todd)
9523e8a Make verify-commits path-independent (Matt Corallo)
f7d4a25 Make verify-commits POSIX-compliant (Matt Corallo)
…accepted blocks.

54326a6 Increase maximum orphan size to 100,000 bytes. (Gregory Maxwell)
8c99d1b Treat orphans as implicit inv for parents, discard when parents rejected. (Gregory Maxwell)
11cc143 Adds an expiration time for orphan tx. (Gregory Maxwell)
db0ffe8 This eliminates the primary leak that causes the orphan map to  always grow to its maximum size. (Gregory Maxwell)
1b0bcc5 Track orphan by prev COutPoint rather than prev hash (Pieter Wuille)
ad0752e Stop trimming when mapTx is empty (Pieter Wuille)
Printing Log without category defined should use LogPrintf

Github-Pull: bitcoin#8230
Meta: PR should have been based on master in the first place
fa58f94 [qa] pull-tester: Start longest test first (MarcoFalke)
fa3b379 [qa] pull-tester: Fix assertion and check for run_parallel (MarcoFalke)
fa32465 [qa] fundrawtransaction: Create get_unspent() (MarcoFalke)
fa8ce3b [qa] assert 'changePosition out of bounds' (MarcoFalke)
Mention ARM executables in the release process documentation
(these were introduced in bitcoin#8188).
As well as that Linux tarballs have changed name to contain an
architecture tuple, instead of `linux32`/`linux64`.
Also mention that `-debug` files should not be uploaded (these were
introduced in bitcoin#8167).
Pulls in the following new languages:

- `af` Afrikaans
- `es_419` Spanish (Latin America)
- `es_AR` Spanish (Argentina)
- `es_CO` Spanish (Colombia)
- `fil` Filipino
- `it_IT` Italian (Italy)
- `ro` Romanian
- `sr@latin` Serbian (Latin)
- `ta` Tamil
- `uz@Latn` Uzbek (Latin)
- `zh_HK` Chinese (Hong Kong)
e5a680d [Doc] Update OS X build notes for 10.11 SDK (fanquake)
3775ff9 Enable mempool consistency checks in unit tests (Pieter Wuille)
… hidden during startup

b3e1348 [Qt] fix a bug where the SplashScreen will not be hidden during startup (Jonas Schnelli)
4cbe05b qt: Periodic transifex update (Wladimir J. van der Laan)
48efec8 Fix some minor compact block issues that came up in review (Matt Corallo)
ccd06b9 Elaborate bucket size math (Pieter Wuille)
0d4cb48 Use vTxHashes to optimize InitData significantly (Matt Corallo)
8119026 Provide a flat list of txid/terators to txn in CTxMemPool (Matt Corallo)
678ee97 Add BIP 152 to implemented BIPs list (Matt Corallo)
56ba516 Add reconstruction debug logging (Matt Corallo)
2f34a2e Get our "best three" peers to announce blocks using cmpctblocks (Matt Corallo)
927f8ee Add ability to fetch CNode by NodeId (Matt Corallo)
d25cd3e Add receiver-side protocol implementation for CMPCTBLOCK stuff (Matt Corallo)
9c837d5 Add sender-side protocol implementation for CMPCTBLOCK stuff (Matt Corallo)
00c4078 Add protocol messages for short-ids blocks (Matt Corallo)
e3b2222 Add some blockencodings tests (Matt Corallo)
f4f8f14 Add TestMemPoolEntryHelper::FromTx version for CTransaction (Matt Corallo)
85ad31e Add partial-block block encodings API (Matt Corallo)
5249dac Add COMPACTSIZE wrapper similar to VARINT for serialization (Matt Corallo)
cbda71c Move context-required checks from CheckBlockHeader to Contextual... (Matt Corallo)
7c29ec9 If AcceptBlockHeader returns true, pindex will be set. (Matt Corallo)
96806c3 Stop trimming when mapTx is empty (Pieter Wuille)
…and notes

06f40ef depends: Mention aarch64 as common cross-compile target (Wladimir J. van der Laan)
05f64c9 doc: Mention Linux ARM builds in release notes (Wladimir J. van der Laan)
b7bf037 doc: Mention ARM executables in release process (Wladimir J. van der Laan)
@NicolasDorier
Copy link
Author

closing for rebased version #101

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.