e5485e8e4be7f2ee0671f58c3dcce35c68ba0ee0 test, bench: make prevector and checkqueue swap member functions noexcept (Jon Atack)
abc1ee509025d92db5311c3f5df3b61c09cad24f validation: make CScriptCheck and prevector swap member functions noexcept (Jon Atack)
Pull request description:
along with those seen elsewhere in the codebase (prevector and checkqueue units/fuzz/bench).
A swap must not fail; when a class has a swap member function, it should be declared noexcept.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c84-a-swap-function-must-not-fail
ACKs for top commit:
pk-b2:
ACK e5485e8e4b
w0xlt:
ACK e5485e8e4b
Tree-SHA512: c82359d5e13f9262ce45efdae9baf71e41ed26568e0aff620e2bfb0ab37a62b6d56ae9340a28a0332c902cc1fa87da3fb72d6f6d6f53a8b7e695a5011f71f7f1
a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc refactor: Remove Node:: queries from GUI (Hennadii Stepanov)
06d519f0b43ed16252428e935d3aeb5a38f582e0 qt: Add SynchronizationState enum to signal parameter (Hennadii Stepanov)
3c709aa69d5bb5a1564c339a0e6a16bac8f02c98 refactor: Remove Node::getReindex() call from GUI (Hennadii Stepanov)
1dab574edf57ccd6cdf5ec706ac328c62142d7a2 refactor: Pass SynchronizationState enum to GUI (Hennadii Stepanov)
2bec309ad6d0f2543948d64ed26f7d9a903f67e5 refactor: Remove unused bool parameter in RPCNotifyBlockChange() (Hennadii Stepanov)
1df77014d8bb733d7d89e36b28671cb47f436292 refactor: Remove unused bool parameter in BlockNotifyGenesisWait() (Hennadii Stepanov)
Pull request description:
This PR is a followup of #18121 and:
- addresses confusion about GUI notification throttling conditions (**luke-jr**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378552386), **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#discussion_r378975960))
- removes `isInitialBlockDownload()` call from the GUI back to the node (on macOS). See: **ryanofsky**'s [comment](https://github.com/bitcoin/bitcoin/pull/18121#pullrequestreview-357730284)
ACKs for top commit:
jonasschnelli:
Core Review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc (modulo [question](https://github.com/bitcoin/bitcoin/pull/18152#pullrequestreview-414140601)).
ryanofsky:
Code review ACK a0d0f1c6c3d736bc0ee076b7f27a0ef59fd260bc. Only changes since last review were rebase and tweaking SynchronizationState enum declaration as suggested (thanks!)
Tree-SHA512: b6a712a710666e763aeee0d5440de1391a4c6c8f7fa661888773e1ba59e9e0f83654ee384d4edc704031be7eb25616e5eca2a6e26058d3efb7f64c47f9ed7316
f685a13bef0418663015ea6d8f448f075510c0ec doc: GetTransaction()/getrawtransaction follow-ups to #22383 (John Newbery)
abc57e1f0882a1a2bb20474648419979af6e383d refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner)
Pull request description:
~This PR is based on #22383, which should be reviewed first~ (merged by now).
In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background.
Relevant IRC log:
```
17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
17:53 <raj_> jnewbery, +1
17:53 <stickies-v> agreed!
17:54 <glozow> jnewbery ya
17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
17:55 <sipa> (before 0.8, validation itself used the txindex)
17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
17:55 <sipa> jnewbery: sure, just providing background
17:56 <sipa> seems very reasonable to move it elsewhere now
```
The commit should be trivial to review with `--color-moved`.
ACKs for top commit:
jnewbery:
Code review ACK f685a13bef0418663015ea6d8f448f075510c0ec
rajarshimaitra:
tACK f685a13bef
mjdietzx:
crACK f685a13bef0418663015ea6d8f448f075510c0ec
LarryRuane:
Code review, test ACK f685a13bef0418663015ea6d8f448f075510c0ec
Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c
78f4c8b98eada337346ffb206339c3ebae4ff43b prefer to use txindex if available for GetTransaction (Jameson Lopp)
Pull request description:
Fixes#22382
Motivation: prevent excessive disk reads if txindex is enabled.
Worth noting that this could be argued to be less of a bug and more of an issue of undefined behavior. If a user calls GetTransaction with the wrong block hash, what should happen?
ACKs for top commit:
jonatack:
ACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
theStack:
Code review ACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
LarryRuane:
tACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
luke-jr:
utACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
jnewbery:
utACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
rajarshimaitra:
Code review ACK 78f4c8b98e
lsilva01:
Code Review ACK and Tested ACK 78f4c8b98e on Ubuntu 20.04
Tree-SHA512: af7db5b98cb2ae4897b28476b2fa243bf7e6f850750d9347062fe8013c5720986d1a3c808f80098e5289bd84b085de03c81a44e584dc28982f721c223651bfe0
e48826ad87b4f92261f7433e84f48dac9bd9e5c3 tests: remove ComputeBlockVersion shortcut from versionbits tests (Anthony Towns)
c5f36725e877d8eb492383844f8ef7535466b366 [refactor] Move ComputeBlockVersion into VersionBitsCache (Anthony Towns)
4a69b4dbe0d7f504811b67c399da7e6d11e4f805 [move-only] Move ComputeBlockVersion from validation to versionbits (Anthony Towns)
0cfd6c6a8f929d5567ac41f95c21548f115efee5 [refactor] versionbits: make VersionBitsCache a full class (Anthony Towns)
8ee3e0bed5bf2cd3c7a68ca6ba6c65f7b9a72cca [refactor] rpc/blockchain.cpp: SoftForkPushBack (Anthony Towns)
92f48f360da5f425428b761219301f509826bec4 deploymentinfo: Add DeploymentName() (Anthony Towns)
ea68b3a5729f5d240e968388c4f88acffeb27228 [move-only] Rename versionbitsinfo to deploymentinfo (Anthony Towns)
c64b2c6a0f79369624ae96b2e3d579d50aae4de6 scripted-diff: rename versionbitscache (Anthony Towns)
de55304f6e7a8b607e6b3fc7436de50910747b0c [refactor] Add versionbits deployments to deploymentstatus.h (Anthony Towns)
2b0d291da8f479739ff394dd92801da8c40b9f8e [refactor] Add deploymentstatus.h (Anthony Towns)
eccd736f3dc231ac0306ca763c3b72cf8247230a versionbits: Use dedicated lock instead of cs_main (Anthony Towns)
36a4ba0aaaa9b35185d7178994e36bc02cca9887 versionbits: correct doxygen comments (Anthony Towns)
Pull request description:
Introduces helper functions to make it easy to bury future deployments, along the lines of the suggestion from [11398](https://github.com/bitcoin/bitcoin/pull/11398#issuecomment-335599326) "I would prefer it if a buried deployment wouldn't require all code paths that check the BIP9 status to require changing".
This provides three functions: `DeploymentEnabled()` which tests if a deployment can ever be active, `DeploymentActiveAt()` which checks if a deployment should be enforced in the given block, and `DeploymentActiveAfter()` which checks if a deployment should be enforced in the block following the given block, and overloads all three to work both with buried deployments and versionbits deployments.
This adds a dedicated lock for the versionbits cache, which is acquired internally by the versionbits functions, rather than relying on `cs_main`. It also moves moves versionbitscache into deploymentstatus to avoid a circular dependency with validation.
ACKs for top commit:
jnewbery:
ACK e48826ad87b4f92261f7433e84f48dac9bd9e5c3
gruve-p:
ACK e48826ad87
MarcoFalke:
re-ACK e48826ad87b4f92261f7433e84f48dac9bd9e5c3 🥈
Tree-SHA512: c846ba64436d36f8180046ad551d8b0d9e20509b9bc185aa2639055fc28803dd8ec2d6771ab337e80da0b40009ad959590d5772f84a0bf6199b65190d4155bed
## Issue being fixed or feature implemented
As discovered during platform testing by @shumkov , it seems as the
chain can halt in miner if somehow mempool would have several
transactions that are somehow invalid (maybe too low fee or something
else). They can't be mined, but miner can't prepare a valid block with
correct Credit Pool amount.
It is indeed can happen although I haven't reproduced it with functional
tests at the moment 🤷♂️
## What was done?
Refactored and simplified a logic of Credit Pool amount of validation
and added one more layer of validation: after all transaction are
actually added to block by miner, it is recalculated one more time.
Also used correct `pindexPrev` instead Tip() for EHF signals.
## How Has This Been Tested?
Before this changes platform failed with this error and chain halt:
```
2023-10-20T06:20:16Z (mocktime: 2023-10-20T06:28:29Z) ERROR: ConnectBlock(DASH): CheckCreditPoolDiffForBlock for block 9d635e1fd0d7a8a5bf16ce158d3a39cbf903864bb6d671769836ea7db6055230 failed with bad-cbtx-asse locked-amount
```
With changes from this PR platform is generate the asset-lock
transactions that are included to block and chain is not halt:
```
2023-10-27T10:45:37Z (mocktime: 2023-10-27T14:37:22Z) GetCreditPoolDiffForBlock: CCreditPool is CCreditPool(locked=32100015, currentLimit=32100015)
```
unit/functional tests are succeed.
## Breaking Changes
N/A; no consensus rules are changed
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
## Issue being fixed or feature implemented
Implementation of accepted proposal:
https://www.dashcentral.org/p/expedite-60-20-20-reallocation
## What was done?
Activates changers brought in #5588 on `v20` hard fork instead of
`mn_rr`.
## How Has This Been Tested?
run tests
## Breaking Changes
Again, Testnet sync is broken
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Currently, the `nSubsidyBase` calculation relies on difficulty. This
leads to variable Block Subsidity.
When Platform will be live, it would constantly require blocks
difficulty in order to calculate the `platformReward` (which relies on
Block Subsidy)
cc @QuantumExplorer
## What was done?
Starting from v20 activation, `nSubsidyBase` will no longer rely on
difficulty and will be constant to 5.
## How Has This Been Tested?
## Breaking Changes
Block rewards will differ.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
this change saw a ~38% performance improvement in header sync reindex
reproduce via `time ./src/qt/dash-qt --nowallet --testnet --reindex
--stopatheight=5`
On Develop this took average of 1:48 to finish, on this branch it took
1:07
## Issue being fixed or feature implemented
Slow header / block validation
## What was done?
Pass around cached block hash
## How Has This Been Tested?
Reindexed testnet
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Implementation of accepted proposal:
https://www.dashcentral.org/p/TREASURY-REALLOCATION-60-20-20
## What was done?
Once Masternode Reward Location Reallocation activates:
- Treasury is bumped to 20% of block subsidy.
- Block reward shares are immediately set to 75% for MN and 25% miners.
(Previous reallocation periods are dropped)
MN reward share should be 75% of block reward in order to represent 60%
of the block subsidy. (according to the proposal)
- `governancebudget` is returned from `getgovernanceinfo` RPC.
## How Has This Been Tested?
`block_reward_reallocation_tests`
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Unlike bitcoin we are using PREVIOUS block in `GetBlockSubsidy()`.
That creates special case for genesis block, because it doesn't have
previous block. In this special case instead of calling
`GetBlockSubsidy` should be used pre-calculated value. To avoid
confusion for new code and simplify implementation, there's introduced a
new method `GetBlockSubsidyPrev` that has other interface: it takes
pointer `CBlockIndex* prev` in agruments instead pair of height + nbits.
These changes are follow-up for #5501
## What was done?
Implemented new method `GetBlockSubsidyPrev()` and used instead of
`GetBlockSubsidy` when it is more convenient.
## How Has This Been Tested?
Run unit/functional tests.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
5021810650afc3073c2af6953ff046ad4d27a1fc Make CanFlushToDisk a const member function (practicalswift)
281cf995547f7683a9e9186bc6384a9fb6035d10 Do not run functions with necessary side-effects in assert() (practicalswift)
Pull request description:
Do not run functions with necessary side-effects in `assert()`.
ACKs for top commit:
laanwj:
Code review ACK 5021810650afc3073c2af6953ff046ad4d27a1fc
sipa:
utACK 5021810650afc3073c2af6953ff046ad4d27a1fc
theStack:
Code Review ACK 5021810650afc3073c2af6953ff046ad4d27a1fc 🟢
Tree-SHA512: 38b7faccc2f16a499f9b7b1b962b49eb58580b2a2bbf63ea49dcc418a5ecc8f21a0972fa953f66db9509c7239af67cfa2f9266423fd220963d091034d7332b96
501e6ab4e778d8f4e95fdc807eeb8644df16203b doc: Add documentation for 'checklevel' argument in 'verifychain' RPC call (Calvin Kim)
Pull request description:
Rationale: When ```bitcoin-cli help verifychain``` is called, the user doesn't get any documentation about the ```checklevel``` argument, leading to issues like #18995.
This PR addresses that issue and adds documentation for what each level does, and that each level includes the checks of the previous levels.
ACKs for top commit:
jonatack:
ACK 501e6ab4e778d8f4e95fdc807eeb8644df16203b `git diff 292ed3c 501e6ab` shows only change since last review is the verifychain RPCHelpMan edit; rebuild and retested manually anyway
MarcoFalke:
ACK 501e6ab4e778d8f4e95fdc807eeb8644df16203b 🚝
Tree-SHA512: 09239f79c25b5c3022b8eb1f76198ba681305d7e8775038e46becffe5f6a14c572e0c5d06b0723fe9d4a015ec42c9f7ca7b80a2a93df0b1b66f5a84a80eeeeb1
## Issue being fixed or feature implemented
During implementation #5469 (master node hard-fork) I noticed that some
parts of `CChainParams` are deprecated and can be removed.
## What was done?
1. removed methods from `CChainParams` that have no implementation at
all:
- UpdateSubsidyAndDiffParams
- UpdateLLMQChainLocks
- UpdateLLMQTestParams
- UpdateLLMQDevnetParams
2. removed method `BIP9CheckMasternodesUpgraded` from `CChainParams` and
a flag `check_mn_protocol` from `versionbitsinfo`.
(to follow-up dashpay/dash#2594)
## How Has This Been Tested?
Run functional/unit tests.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Many objects created and functions called by passing `const
std::unique_ptr<Obj>& obj` instead directly passing `Obj& obj`
In some cases it is indeed needed, but in most cases it is just extra
complexity that is better to avoid.
Motivation:
- providing reference to object instead `unique_ptr` is giving warranty
that there's no `nullptr` and no need to keep it in mind
- value inside unique_ptr by reference can be changed externally and
instead `nullptr` it can turn to real object later (or in opposite)
- code is shorter but cleaner
Based on that this refactoring is useful as it reduces mental load when
reading or writing code.
`std::unique` should be used ONLY for owning object, but not for passing
it everywhere.
## What was done?
Replaced most of usages `std::unique_ptr<Obj>& obj` to `Obj& obj`.
Btw, in several cases implementation assumes that object can be nullptr
and replacement to reference is not possible.
Even using raw pointer is not possible, because the empty
std::unique_ptr can be initialized later somewhere in code.
For example, in `src/init.cpp` there's called `PeerManager::make` and
pass unique_ptr to the `node.llmq_ctx` that would be initialized way
later.
That is out of scope this PR.
List of cases, where reference to `std::unique_ptr` stayed as they are:
- `std::unique_ptr<LLMQContext>& llmq_ctx` in `PeerManagerImpl`,
`PeerManager` and `CDSNotificationInterface`
- `std::unique_ptr<CDeterministicMNManager>& dmnman` in
`CDSNotificationInterface`
Also `CChainState` have 3 references to `unique_ptr` that can't be
replaced too:
- `std::unique_ptr<llmq::CChainLocksHandler>& m_clhandler;`
- `std::unique_ptr<llmq::CInstantSendManager>& m_isman;`
- `std::unique_ptr<llmq::CQuorumBlockProcessor>&
m_quorum_block_processor;`
## How Has This Been Tested?
Run unit/functional tests.
## Breaking Changes
No breaking changes, all of these changes - are internal APIs for Dash
Core developers only.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
e09c701e0110350f78366fb837308c086b6503c0 scripted-diff: Bump copyright of files changed in 2020 (MarcoFalke)
6cbe6209646db8914b87bf6edbc18c6031a16f1e scripted-diff: Replace CCriticalSection with RecursiveMutex (MarcoFalke)
Pull request description:
`RecursiveMutex` better clarifies that the mutex is recursive, see also the standard library naming: https://en.cppreference.com/w/cpp/thread/recursive_mutex
For that reason, and to avoid different people asking me the same question repeatedly (e.g. https://github.com/bitcoin/bitcoin/pull/15932#pullrequestreview-339175124 ), remove the outdated alias `CCriticalSection` with a scripted-diff
2e42050b7fc61201f202438e8cd4383a06eb98d5 doc: fix undo data filename (s/undo???.dat/rev???.dat/) (Sebastian Falbesoner)
Pull request description:
This typo was discovered in the course of a review club to #20827, see https://bitcoincore.reviews/20827#l-31.
ACKs for top commit:
shaavan:
ACK 2e42050b7fc61201f202438e8cd4383a06eb98d5
Tree-SHA512: 0c7a00dce24c03bee6d37265d5b4bc97e976c3f3910af1113f967f6298940f892d6fb517f7b154f32ccedb365060314d4d78d5eb2a9c68b25f0859a628209cd3
3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5bb1e37b2e6ab2c52791ac05d9271238 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b944f6ad51b3c895837729d3aa56eea [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b3174d19a6d8691ae07e92b32fdfaef11 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e49246822421a7bcc720491427e1dba8a3 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed9afbe5a96caa5f0f4cbec730d27460 [validation] Add CValidationState subclasses (John Newbery)
Pull request description:
Carries out some remaining tidy-ups remaining after PR 15141:
- split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
- various minor code style tidy-ups to the ValidationState class
- remove the useless `ret` parameter from `ValidationState::Invalid()`
- remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
- remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.
Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:
Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.
```sh
git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^
```
After that it's possible to easily see the mechanical changes with:
```sh
git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
```
ACKs for top commit:
laanwj:
ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf
amitiuttarwar:
code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
9075d13153ce06cd59a45644831ecc43126e1e82 [docs] Add release notes for removal of REJECT reasons (John Newbery)
04a2f326ec0f06fb4fce1c4f93500752f05dede8 [validation] Fix REJECT message comments (John Newbery)
e9d5a59e34ff2d538d8f5315efd9908bf24d0fdc [validation] Remove REJECT code from CValidationState (John Newbery)
0053e16714323c1694c834fdca74f064a1a33529 [logging] Don't log REJECT code when transaction is rejected (John Newbery)
a1a07cfe99fc8cee30ba5976dc36b47b1f6532ab [validation] Fix peer punishment for bad blocks (John Newbery)
Pull request description:
We no longer send BIP 61 REJECT messages, so there's no need to set
a REJECT code in the CValidationState object.
Note that there is a minor bug fix in p2p behaviour here. Because the
call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only
previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`,
then there are cases were `MaybePunishNode()` can get called where it
wasn't previously:
- when `AcceptBlockHeader()` fails with `CACHED_INVALID`.
- when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`.
Note that `BlockChecked()` cannot fail with an 'internal' reject code. The
only internal reject code was `REJECT_HIGHFEE`, which was only set in
ATMP.
This reverts a minor bug introduced in 5d08c9c579.
ACKs for top commit:
ariard:
ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits
fjahr:
ACK 9075d13153ce06cd59a45644831ecc43126e1e82, confirmed diff to last review was fixing nits in docs/comments.
ryanofsky:
Code review ACK 9075d13153ce06cd59a45644831ecc43126e1e82. Only changes since last review are splitting the main commit and updating comments
Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7
b6aadcd5b4350a6ebcd57e88e7a0853cedf7c2fb build: Add -Werror=mismatched-tags (Hennadii Stepanov)
1485124291368c4a2ca8ea09c18e813f1dbabf5c Fix -Wmismatched-tags warnings (Hennadii Stepanov)
Pull request description:
Warnings were introduced in #20749:
```
./validation.h:43:1: warning: class 'CCheckpointData' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags]
class CCheckpointData;
^
./chainparams.h:24:8: note: previous use is here
struct CCheckpointData {
^
./validation.h:43:1: note: did you mean struct here?
class CCheckpointData;
^~~~~
struct
1 warning generated.
```
This change fixes AppVeyor build: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37547435
ACKs for top commit:
glozow:
utACK b6aadcd5b4🚗
practicalswift:
cr ACK b6aadcd5b4350a6ebcd57e88e7a0853cedf7c2fb: patch looks correct
Tree-SHA512: 3ac887ebdbf9a1ae33c1fd5381b3b8d83388ad557ddeb55013acd42bb9752a5bd009e3a0eed52644a023a7a0dda1c159277981af82f58fb0abfe60b84e01bf29
fafb381af8279b2d2ca768df0bf68d7eb036a2f9 Remove mempool global (MarcoFalke)
fa0359c5b30730744aa8a7cd9ffab79ded91041f Remove mempool global from p2p (MarcoFalke)
eeee1104d78eb59a582ee1709ff4ac2c33ee1190 Remove mempool global from init (MarcoFalke)
Pull request description:
This refactor unlocks some nice potential features, such as, but not limited to:
* Removing the fee estimates global (would avoid slightly fragile workarounds such as #18766)
* Making the mempool optional for a "blocksonly" operation mode
Even absent those features, the new code without the global should be easier to maintain, read and write tests for.
ACKs for top commit:
jnewbery:
utACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9
hebasto:
ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9, I have reviewed the code and it looks OK, I agree it can be merged.
darosior:
ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9
Tree-SHA512: a2e696dc377e2e81eaf9c389e6d13dde4a48d81f3538df88f4da502d3012dd61078495140ab5a5854f360a06249fe0e1f6a094c4e006d8b5cc2552a946becf26
