8f2d7737cc236b6122f30e31856eb3181960fba1 test: add functional test for non-standard txs with too large scriptSig (Sebastian Falbesoner)
Pull request description:
Approaches another missing functional test of issue #17394 (counterpart to unit test in PR #17480, Commit 5e8a56348b): A transaction is rejected by the mempool with reason `"scriptsig-size"` if any of the inputs' scriptSig is larger than 1650 bytes.
ACKs for top commit:
MarcoFalke:
ACK 8f2d7737cc236b6122f30e31856eb3181960fba1
instagibbs:
ACK 8f2d7737cc
Tree-SHA512: 7a45b8a4181158be3e3b91756783ddf032f132ca8780dc35fac91b2df2149268f784d28ac56005135c4d86a357c57805c5a54b8155f0d049932844b18dc03992
5e8a56348b5e1026e9ddcae0b2fa2a68faf4439e test: add unit test for non-standard txs with too large scriptSig (Sebastian Falbesoner)
Pull request description:
Approaches the first missing test of issue #17394: Checks that the function `IsStandardTx()` returns rejection reason `"scriptsig-size"` if any one the inputs' scriptSig is larger than 1650 bytes.
ACKs for top commit:
MarcoFalke:
ACK 5e8a56348b5e1026e9ddcae0b2fa2a68faf4439e
instagibbs:
ACK 5e8a56348b
Tree-SHA512: 79977b12ddea9438a37cefdbb48cc551e4ad02a8ccfaa2d2837ced9f3a185e2e07cc366c243b9e3c7736245e90e315d7b4110efc6b440c63dbef7ee2c9d78a73
297e09855793feb94c3229ed989bef8b1eac864e Fix doxygen errors (Ben Woosley)
Pull request description:
These are all the remaining errors identified via -Werror=documentation, e.g.:
```
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
```
You can use this to run with `-Wdocumentation` yourself: #14920
ACKs for top commit:
laanwj:
ACK 297e09855793feb94c3229ed989bef8b1eac864e
Tree-SHA512: a232d893b170873d923e77fa56c56a6567e7fd120b5af1f52cfeeae1093eec55621604cc80a523678f6fedc8bbb31228c4aa8dc2a630ce9ffc91525988522af7
e1c582cbaa4c094d204da34c3b1fdd0d4c557519 contrib: makeseeds: Read suspicious hosts from a file instead of hardcoding (Sanjay K)
Pull request description:
referring to: https://github.com/bitcoin/bitcoin/issues/17020
good first issue: reading SUSPICIOUS_HOSTS from a file.
I haven't changed the base hosts that were included in the original source, just made it readable from a file.
ACKs for top commit:
practicalswift:
ACK e1c582cbaa4c094d204da34c3b1fdd0d4c557519 -- diff looks correct
Tree-SHA512: 18684abc1c02cf52d63f6f6ecd98df01a9574a7c470524c37e152296504e2e3ffbabd6f3208214b62031512aeb809a6d37446af82c9f480ff14ce4c42c98e7c2
1be0b1fb2adcf95d76f879195564c0bf84162e31 test: add functional test for non-standard bare multisig txs (Sebastian Falbesoner)
Pull request description:
Approaches another missing functional test of issue #17394 (counterpart to unit test in PR #17502): A transaction is rejected by the mempool with reason `"bare-multisig"` if any of the outputs' scriptPubKey has bare multisig format (`M <PubKey1> <PubKey2> ... <PubKeyN> N OP_CHECKSIG`) and bitcoind is started with the argument `-permitbaremultisig=0`.
ACKs for top commit:
instagibbs:
utACK 1be0b1fb2a
kristapsk:
ACK 1be0b1fb2adcf95d76f879195564c0bf84162e31
Tree-SHA512: 2cade68c4454029b62278b38d0f137c2605a0e4450c435cdda2833667234edd4406f017ed12fa8df9730618654acbaeb68b16dcabb9f5aa84bad9f1c76c6d476
831e1220bc151b1016412359775406b34cb8f52c build: remove double LIBBITCOIN_SERVER linking (fanquake)
Pull request description:
Seems that this is no longer required. Have tested building on macOS and Debian.
ACKs for top commit:
promag:
ACK 831e1220bc151b1016412359775406b34cb8f52c.
practicalswift:
ACK 831e1220bc151b1016412359775406b34cb8f52c
laanwj:
ACK 831e1220bc151b1016412359775406b34cb8f52c
Tree-SHA512: d226d9fa0292189fae7e2af14781a511c3633f1352324f19ae642e941d06c34e2abf8b1df97d2330d76dba6024a93d8d341e02cc4882d7066f97e82585631fe1
168b781fe7f3f13b24c52a151f36de4cdd0a340a Continue relaying transactions after they expire from mapRelay (Anthony Towns)
Pull request description:
This change allows peers to request transactions even after they've expired from mapRelay and even if they're not doing mempool requests. This is intended to allow for CPFP of old transactions -- if parent tx P wasn't relayed due to low fees, then a higher fee rate child C is relayed, peers will currently request the parent P, but we prior to this patch, we will not relay it due to it not being in mapRelay.
ACKs for top commit:
MarcoFalke:
re-ACK 168b781fe7f3f13b24c52a151f36de4cdd0a340a (only change is comment fixup)
sdaftuar:
re-ACK 168b781fe7f3f13b24c52a151f36de4cdd0a340a
sipa:
ACK 168b781fe7f3f13b24c52a151f36de4cdd0a340a
Tree-SHA512: b206666dd1450cd0a161ae55fd1a7eda2c3d226842ba27d91fe463b551fd924b65b92551b14d6786692e15cf9a9a989666550dfc980b48ab0f8d4ca305bc7762
fa40e48c50d8ccf42ce5e66c12390e2ed4b60e75 ci: Remove unparseable lines from supp file for old xenial clang tsan (MarcoFalke)
fa1bfc476c9208a4c412c8ca74d05f52bb47766f ci: ubsan report_error_type=1 and add suppressions (MarcoFalke)
fa69cef13e5aab8264339eb3d50a9e89d59efd87 test: Print stderr when subprocess fails (MarcoFalke)
2222c305866a77065ab5be24c1c252bae252bb59 test: Use char instead of unsigned char (MarcoFalke)
faa8023ce9a47b282e1fac3ca8b3a7bb0042935a ci: Bump to clang-8 for asan build to avoid segfaults on ppc64le (MarcoFalke)
Pull request description:
Use clang-8 instead of default clang (which is clang-6 on Bionic) to avoid spurious segfaults when running the ci system on ppc64le
ACKs for top commit:
practicalswift:
ACK fa40e48c50d8ccf42ce5e66c12390e2ed4b60e75 assuming Travis is happy -- diff looks correct :)
Tree-SHA512: f4f26232d3a0ef38da245869340f723d279a3db9823befbc735fb5a00024dae041c7306d7ae55d2488e6f86aa96cdea155b007aefb561fba505141e8dbc717dc
It's incorrect to convert between different enum classes using
initialisation.
However, it's OK to convert between different enum classes using the
same underlying type using static_cast.
Signed-off-by: Oleg Girko <ol@infoserver.lv>
Signed-off-by: Oleg Girko <ol@infoserver.lv>
Co-authored-by: Oleg Girko <ol@infoserver.lv>
acf8abc7f3cf7efa418a46f9f69f23f1a5035582 gui: Fix unintialized WalletView::progressDialog (João Barbosa)
Pull request description:
#17911 shows that it's possible to read the unintialized `progressDialog` in f32564f0a7/src/qt/walletview.cpp (L296-L297).
And the debugger shows
```
(gdb) bt
#0 0x0000555556687c60 in QProgressDialog::wasCanceled() const ()
#1 0x000055555572989f in WalletView::showProgress (this=0x5555577d7a70,
title=..., nProgress=1) at qt/walletview.cpp:322
```
Closes#17911.
ACKs for top commit:
hebasto:
ACK acf8abc7f3cf7efa418a46f9f69f23f1a5035582, I have reviewed the code and it looks OK, I agree it can be merged.
elichai:
utACK acf8abc7f3cf7efa418a46f9f69f23f1a5035582
kristapsk:
ACK acf8abc7f3cf7efa418a46f9f69f23f1a5035582
MarcoFalke:
ACK acf8abc7f3cf7efa418a46f9f69f23f1a5035582
Tree-SHA512: f5e6d873192d08d1a572e66e17c2e06d1ce27d01aa196b2a7ed591008641295bb02cda8ac90919ff2d2fc778316c2e143f8d36599e0d377779758853dfaf0a31
900d8f6f70859f528e84c5c38d0332f81d19df55 util: Disallow network-qualified command line options (Russell Yanofsky)
Pull request description:
Previously these were allowed but ignored.
This change implements one of the settings simplifications listed in #17508. Change includes release notes.
ACKs for top commit:
laanwj:
ACK 900d8f6f70859f528e84c5c38d0332f81d19df55
Tree-SHA512: ab020a16a86c1e8ec709fbf798d533879d32c565eceeb7eb785c33042c49c6b4d1108c5453d8166e4a2abffc2c8802fbb6d3b895e0ddeefa8f274fd647e3c8ad
ff59bcd3213ef61f2167c0aa60fcaf5afbc20c61 gui: Drop PeerTableModel dependency to ClientModel (João Barbosa)
Pull request description:
Class `PeerTableModel` doesn't actually depend on `ClientModel`.
ACKs for top commit:
Empact:
Code Review ACK ff59bcd321
hebasto:
ACK ff59bcd3213ef61f2167c0aa60fcaf5afbc20c61, tested on Linux Mint 19.3. No changes in behavior are observed.
Tree-SHA512: 29fa3c316c05b8f7b9340e5859bbb8c3a0b826aa7c865c892cfa13b5ad30f822fcaae4e01555f7860cd1727f20b7ef555a808235522a04a6eebaaa7b605f8595
0a50019fde7781263e0c8f041d1d9dcb0dee77e8 Walk pindexBestHeader back to ChainActive().Tip() if it is invalid (Matt Corallo)
Pull request description:
Instead of keeping pindexBestHeader set to the best header we've
ever seen, reset it back to our validated tip if we find an ancestor
of it turns out to be invalid. While the name is now a bit confusing,
this matches much better with how it is used in practice, see below.
Further, this opens up more use-cases for it in the future, namely
aggressively searching for new peers in case we have discovered
(possibly via some covert channel) headers which we do not know to be
invalid, but which we cannot find block data for.
Places pindexBestHeader is used:
* Various GUI displays of the best header and getblockchaininfo["headers"],
I don't think changing this is bad, and if anything this is less confusing
in the presence of an invalid block.
* IsCurrentForFeeEstimation(): If anything I think ensuring pindexBestHeader
isn't some crazy invalid chain is better than the alternative, even in the
case where you are rejecting the current chain due to hardware error (since
hopefully in that case you won't get any new blocks anyway).
* ConnectBlock assumevalid checks: We use pindexBestHeader to check that the
block we're connecting leads to something with nMinimumChainWork (preventing
a user-set assumevalid from having bogus work) and that the block we're
connecting leads to pindexBestHeader (I'm not too worried about this one -
it's nice to "disable" assumevalid if we have a long invalid headers chain,
but I don't see it as a critical protection).
* BlockRequestAllowed() uses pindexBestHeader as its target to ensure the
requested block is within a month of the "current chain". I don't think this
is a meaningful difference, if we're rejecting the current tip we're
trivially fingerprintable anyway, and if the chain really does have a bunch
of invalid crap near the tip, using the best not-invalid header is likely a
better criteria.
* ProcessGetBlockData uses pindexBestHeader as the "current chain" definition
of whether a block request is "historical" for the purpose of bandwidth
limiting. Similarly, I don't see why this is a meaningful change.
* We use pindexBestHeader for requesting missing headers on receipt of a
headers/compact block message or block inv as well as for initial getheaders.
I think this is definitely wrong, using the best not-invalid header for such
requests is much better.
* We use pindexBestHeader to define the "current chain" for deciding when
we're close to done with initial headers sync. I don't think this is a
meaningful change.
* We use pindexBestHeader to decide if initial headers sync has timed out. If
we're rejecting the chain due to hardware error this may result in
additional cases where we ban a peer, but this is already true, so I think
its fine.
ACKs for top commit:
fjahr:
ACK 0a50019fde7781263e0c8f041d1d9dcb0dee77e8
kallewoof:
ACK 0a50019fde7781263e0c8f041d1d9dcb0dee77e8
ariard:
utACK 0a50019
Tree-SHA512: 2ecfa973a9878a00313ae7ede94a9bd7710e0caf55b544b10bbc46dc463a0478cbaf477e6cdd072356d5a0c5fb3848e9339284af785a2995c20bae8bd23f23e5
9a299a59cc8a9ab516e047356c5bc0e93774b557 net: reference instead of copy in BlockConnected range loop (Jon Atack)
Pull request description:
Reference elements in range for loop instead of copying them and
fix Clang `-Wrange-loop-analysis` warning introduced in a029e18
```
net_processing.cpp:1185:25: warning: loop variable 'ptx' of
type 'const std::shared_ptr<const CTransaction>' creates a copy from
type 'const std::shared_ptr<const CTransaction>' [-Wrange-loop-analysis]
for (const auto ptx : pblock->vtx) {
^
net_processing.cpp:1185:14: note: use reference type
'const std::shared_ptr<const CTransaction> &' to prevent copying
for (const auto ptx : pblock->vtx) {
^~~~~~~~~~~~~~~~
1 warning generated.
```
ACKs for top commit:
Empact:
ACK 9a299a59cc
MarcoFalke:
ACK 9a299a59cc8a9ab516e047356c5bc0e93774b557
promag:
ACK 9a299a59cc8a9ab516e047356c5bc0e93774b557.
elichai:
ACK 9a299a59cc8a9ab516e047356c5bc0e93774b557
emilengler:
ACK 9a299a5.
Tree-SHA512: 9284d1b00684877505454a05071212758c8cea083534e2eec09bfc8a9c3059eea811d2008f6a5a678539444f0d5b3134db1bd23da6514b3d3a1440634c8b53be
cb8a86d9f952401eaad68b2e3818ce50f7befd91 gui: Remove WalletView and BitcoinGUI circular dependency (João Barbosa)
ac3d10777d65b68862c6deb57594c8fc4d21ca77 gui: Add transactionClicked and coinsSent signals to WalletView (João Barbosa)
Pull request description:
Essentially moves the code in `WalletView::setBitcoinGUI` to the only caller. Two new signals are added beforehand in the first commit so that the connections in `WalletFrame` are all from the wallet view.
ACKs for top commit:
hebasto:
ACK cb8a86d9f952401eaad68b2e3818ce50f7befd91, tested on Linux Mint 19.3.
jonasschnelli:
utACK cb8a86d9f952401eaad68b2e3818ce50f7befd91
Tree-SHA512: 250316cd3689e51c8cded9ccd75963c836dcafa6db25d684f2aa691dea9738895f9140793e0f925784909e39f8257f7e1c7d611e8bd6d6634e1a50333f4ddb1e
3aee10b80b9d9a0f5172fc2ee75f03a37d5c3863 gui: Drop ShutdownWindow dependency to BitcoinGUI (João Barbosa)
61eb058cc10592cfa314ba2209fb370706100e8b gui: Drop BanTableModel dependency to ClientModel (João Barbosa)
Pull request description:
`ShutdownWindow::showShutdownWindow` just needs a widget to center the shutdown window and to borrow its title.
ACKs for top commit:
hebasto:
ACK 3aee10b80b9d9a0f5172fc2ee75f03a37d5c3863, since previous review only suggested change `QWidget` --> `QMainWindow`
jonasschnelli:
utACK 3aee10b80b9d9a0f5172fc2ee75f03a37d5c3863
Tree-SHA512: e15cb6ee274730bd071d3d97b540c5059e5c655248d69a37c3fd00f2aacc6cfcb36b9a65755718027e15482ec8e5e85534c1dc13d0ddb4e0680df03fbf6571f2
a029e18c2bf67dd00552b0f4bbc85fa2fa5b973b Use rolling bloom filter of recent block tx's for AlreadyHave() check (Suhas Daftuar)
Pull request description:
In order to determine whether to download or process a relayed transaction, we first try to check whether we already have the transaction -- either in the mempool, in our filter of recently rejected transactions, in our orphan pool, or already confirmed in a block.
Prior to this commit, the heuristic for checking whether a transaction was confirmed in a block is based on whether there's a coin cache entry corresponding to the 0- or 1-index vout of the tx. While that is a quick check, it is very imprecise (eg if those outputs were already spent in another block, we wouldn't detect that the transaction has already been confirmed) -- we can do better by just keeping a rolling bloom filter of the transactions in recent blocks, which will better capture the case of a transaction which has been confirmed and then fully spent.
This should reduce the bandwidth that we waste by requesting transactions which will not be accepted to the mempool.
To avoid relay problems for transactions which have been included in a recent block but then reorged out of the chain, we clear the bloom filter whenever a block is disconnected.
ACKs for top commit:
MarcoFalke:
re-ACK a029e18c2b only stylistic and comment fixups 🍴
sipa:
utACK a029e18c2bf67dd00552b0f4bbc85fa2fa5b973b
jonatack:
Code review ACK a029e18c2bf67dd00552b0f4bbc85fa2fa5b973b also built/ran tests and am running bitcoind with mempool debug logging and custom logging. Looked a bit into CRollingBloomFilter and also the mempool median time past checks mentioned above; I don't have a deep understanding of those areas yet but the concept here and changes LGTM. Tests and other optimisations could be added as a follow-up. In favor of seeing this move forward if no major immediate concerns.
Tree-SHA512: 784c9a35bcd3af5db469063ac7d26b4bac430e451e5637a34d8a538c3ffd1433abdd3f06e5584e7a84bfa9e791449e61819397b5a6c7890fa59d78ec3ba507b2
f41d58966995fe69df433fa684117fae74a56e66 Document better -keypool as a look-ahead safety mechanism (Antoine Riard)
Pull request description:
If after a backup, an address is issued beyond the initial
keypool range and none of the addresses in this range
is seen onchain, if a wallet is restored from backup, even in
case of rescan, funds may be loss due to the look-ahead
buffer not being incremented and so restored wallet not detecting
onchain out-of-range address as derived from its seed.
This scenario is theoretically unavoidable due to the requirement
of the keypool to have a max size. However, given the default
keypool size, this is unlikely. Document better keypool size
implications to avoid user setting a too low value.
While reviewing #17681, it took me a while to figure out the safety implications of keypool, I find it would be better to document this a bit farther to avoid users shooting themselves in the foot. For further context & discussion, see https://github.com/bitcoin/bitcoin/pull/17681#issuecomment-563613452
ACKs for top commit:
ryanofsky:
Code review ACK f41d58966995fe69df433fa684117fae74a56e66. Just "Warning:" prefix added since the last review
jonatack:
ACK f41d58966995fe69df433fa684117fae74a56e66 code review and build/test. The added `Warning:` since last review is a good addition.
Tree-SHA512: d3d0ee88fcdfc5c8841a2bd4bada0e4eeb412a0dce5054e5fb023643c2fa57206a0f3efb06890c245528dc4431413ed2fd5645b9319d26245d044c490b7f0db0
* block_connected: don't serialize block hash twice
In the validation:block_connected tracepoint, we call block->GetHash(),
which ends up calling CBlockHeader::GetHash(), executing around 8000
serialization instructions. We don't need to do this extra work, because
block->GetHash() is already called further up in the function. Let's
save that value as a local variable and re-use it in our tracepoint so
there is no unnecessary tracepoint overhead.
Signed-off-by: William Casarin <jb55@jb55.com>
* ConnectBlock: re-use hash on budget start
Signed-off-by: William Casarin <jb55@jb55.com>
Co-authored-by: William Casarin <jb55@jb55.com>
* refactor: make ThreadOpenMasternodeConnections much more readable, using lambdas!
* refactor: use std::chrono::ms more clearly
* fix log
* drop unneeded line
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* style: fix whitespace
* Apply suggestions from code review
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* tests: extend "age" period in `feature_llmq_connections.py`
see `NOTE`
* tests: sleep more in `wait_until` by default
Avoid overloading rpc with 20+ requests per second, 2 should be enough.
* tests: various fixes in `activate_dip0024`
- lower batch size
- no fast mode
- disable spork17 while mining
- bump mocktime on every generate call
* tests: bump mocktime on generate in `activate_dip8`
* tests: fix `reindex` option in `restart_mn`
Make sure nodes actually finished reindexing before moving any further.
* tests: trigger recovery threads and wait on mn restarts
* tests: sync blocks in `wait_for_quorum_data`
* tests: bump disconnect timeouts in `p2p_invalid_messages.py`
1 is too low for busy nodes
* tests: Wait for addrv2 processing before bumping mocktime in p2p_addrv2_relay.py
* tests: use `timeout_scale` option in `get_recovered_sig` and `isolate_node`
* tests: fix `wait_for...`s
* tests: fix `close_mn_port` banning test
* Bump MASTERNODE_SYNC_RESET_SECONDS to 900
This helps to avoid issues with 10m+ bump_mocktime on isolated nodes in feature_llmq_is_retroactive.py and feature_llmq_simplepose.py.
* style: fix extra whitespace
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
It's incorrect to convert between different enum classes using
initialisation.
However, it's OK to convert between different enum classes using the
same underlying type using static_cast.
Signed-off-by: Oleg Girko <ol@infoserver.lv>
Signed-off-by: Oleg Girko <ol@infoserver.lv>
Co-authored-by: Oleg Girko <ol@infoserver.lv>
