Fix wrong ref counting #1358
Fix wrong ref counting #1358
Conversation
in case of multiple inserts at same height.
| if self | ||
| .candidates_by_block_number | ||
| .entry(block_number) | ||
| .or_default() | ||
| .insert(candidate_hash); | ||
| .insert(candidate_hash) | ||
| { | ||
| self.candidates.insert(candidate_hash); | ||
| } |
There was a problem hiding this comment.
How is this different from previous logic? I don't really understand, it should have worked for multiple blocks per height previously. This if will fail only in case of insertion of the same CandidateHash, which shouldn't happen, but even then it would be the same as previously.
There was a problem hiding this comment.
Oh, it's because of ref counting. nvm
There was a problem hiding this comment.
No, because the inserts in candidates is ref counted. Therefore if we insert at the same height multiple times, we would increase the refcount each time. But in cleanup we would only decrement it once, hence we would never clean it.
Demonstration: The modified test fails without that change (pruning does not happen).
In case of forks there can easily be the same candidate included twice at the same height.
There was a problem hiding this comment.
But this will only happen if we insert the same candidate (hash), not different candidates at the same height, no? How is this possible?
There was a problem hiding this comment.
I guess if there are multiple forks at the same height including the same candidate.
There was a problem hiding this comment.
Yes same hash, same height.
- B
A<
- B'
A candidate gets backed in A, afterwards we get a fork. Bitfields will be imported in both forks. Result: same candidate included at the same height.
There was a problem hiding this comment.
This is really nasty. Good catch!
* master: (25 commits) Markdown linter (#1309) Update `fmt` file and some authors (#1379) Bump the known_good_semver group with 1 update (#1375) Bump proc-macro-warning from 0.4.1 to 0.4.2 (#1376) feat: add futures api to `TransactionPool` (#1348) Ensure cumulus/bridges is ignored by formatter and run it (#1369) substrate: chain-spec paths corrected in zombienet tests (#1362) contracts: Update to wasmi 0.31 (#1350) [improve docs]: Template pallet (#1280) [xcm-emulator] Unignore cumulus integration tests (#1247) Fix wrong ref counting (#1358) Use cached session index to obtain executor params (#1190) fix typos (#1339) Use bandersnatch-vrfs with locked dependencies ref (#1342) Bump bs58 from 0.4.0 to 0.5.0 (#1293) Contracts: `seal0::balance` should return the free balance (#1254) Logs: add extra debug log for negative rep changes (#1205) Added short-benchmarks for cumulus (#1183) [xcm-emulator] Improve hygiene and clean up (#1301) Bump the known_good_semver group with 1 update (#1347) ...
* Fix wrong ref counting in case of multiple inserts at same height. * More warn --------- Co-authored-by: eskimor <eskimor@no-such-url.com>
|
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/stalled-parachains-on-kusama-post-mortem/3998/1 |
in case of multiple inserts at same height.
More tests and sanity checking coming, but this is definitely wrong and also very likely the culprit we saw on Kusama.