Backport triv pr15 #4260
Merged
Backport triv pr15 #4260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PastaPastaPasta
commented
Jul 15, 2021
UdjinM6
requested changes
Jul 16, 2021
04a69c2 macOS deploy: use the new plistlib API (Jonas Schnelli) Pull request description: See https://docs.python.org/3/library/plistlib.html. The old API was deprecated in 3.4 and removed in 3.9. ~~AFAIK the macdeployplus scripts is only used when calling `make deploy` locally (on macOS). The linux cross compile build (like gitian) are not affected by this PR.~~ ACKs for top commit: fanquake: ACK 04a69c2 - I checked that `make deploy` on macOS currently fails when building master and using Python 3.9. This PR fixes that, and it's fine to use (and backport) these changes as they only require Python 3.4. Related note: I think we could just about drop our native_biplist dependency entirely given some changes upstream. practicalswift: ACK 04a69c2: patch looks correct Tree-SHA512: c5bb60c5157b371d680c82e0978470a488f3edc58cd09e1be635fed59420f227dd113e901c28e15a463da6fe81dc64d08a701b1fdfeb4502f418785707dbebbc
…Printf 440f8d3 fix potential devision by 0 (Jonas Schnelli) Pull request description: bitcoin#20344 removed the divide-by-zero sanitizer suppression in `wallet/wallet.cpp` but kept a potential devision by zero in `wallet.cpp`'s fee logging. Detected here https://bitcoinbuilds.org/index.php?job=ffb7d59f-379f-4f27-a273-a5595b8c5f07 ACKs for top commit: practicalswift: ACK 440f8d3 laanwj: Code review ACK 440f8d3 hebasto: re-ACK 440f8d3 Tree-SHA512: 9f7903d1e567497c5f972d39e9629c059151e705dbed0a6b88f7c6650c50ecf820f78e3e0f3e629c661d45a938c5d7659faae7c61e47ca8b3bdb029661bca55a
…en version is not 3 d355a30 Break circuit earlier (lontivero) Pull request description: Currently when parsing an onion v3 address the pubic key checksum is calculated in order to compare it with the received address checksum. However this step is not necessary if the address version byte is not 3, in which case the method can return with false immediately. ACKs for top commit: jonatack: ACK d355a30 practicalswift: ACK d355a30 -- patch looks correct hebasto: ACK d355a30, I have reviewed the code and it looks OK, I agree it can be merged. sipa: utACK d355a30 Tree-SHA512: 9e4506793b7f4a62ce8edc41a260a8c125ae81ed2f90cd850eb2a9214d323c446edc7586c7b0590dcbf3aed5be534718b77bb19c45b48f8f52553d32a3663a65
d9141a0 doc: clarify CRollingBloomFilter size estimate (Anthony Towns) Pull request description: Based on bitcoin#19130, this change improves the comment for `CRollingBloomFilter` in `bloom.h`: - Give examples to illustrate the heuristic "1.8 bytes per element per factor 0.1 of false positive rate" - Add some Python code which can be copy/pasted for convenient filter size calculation (in an interpreter) - Reconcile the newly added code with the existing approximation ACKs for top commit: laanwj: ACK d9141a0 Tree-SHA512: e7138b3c531883a750ead06368975c750863fde7ef6f2633b137eca011079226e9205316217322014399fba05a48f294c788dd700bb7d479c58fe1f23e40419f
5c3eaf9 doc: Add warnings for http interfaces limitations (Fabian Jahr) Pull request description: `libevent`, which is used for our rest interface, can use up all of the available file descriptors in a system if too many connections are opened at once. If a new block is connected at the same time and can not be written to disk because there are no file descriptors available, the node crashes. Based on my investigation so far the issue is best solved upstream which means we have to wait for the next release (2.2). In the meantime it would be good if we would warn users of this limitation. See bitcoin#11368 for more background. ACKs for top commit: MarcoFalke: ACK 5c3eaf9 Tree-SHA512: 73914538588477ead19068f5832fdcc8e0eb736e51f73b3aca501c93165e5ad634c2511a3fcffff251adcd3efda23a742b48211ad9d3b2a29cdeac17201d06a1
…long lines aa929ab [docs] Update developer notes to discourage very long lines (John Newbery) Pull request description: Mandatory rules on line lengths are bad - there will always be cases where a longer line is more readable than the alternative. However, very long lines for no good reason _do_ hurt readability. For example, this declaration in validation.h is 274 chars: ```c++ bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs); ``` That won't fit on one line without wrapping on my 27" monitor with a comfortable font size. Much easier to read is something like: ```c++ bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs); ``` Therefore, _discourage_ (don't forbid) line lengths greater than 100 characters in our developer style guide. 100 chars is somewhat arbitrary. The old standard was 80, but that seems very limiting with modern displays. ACKs for top commit: fanquake: ACK aa929ab - this is basically just something to point too when a PR has unreasonably long lines for no particularly reason. practicalswift: ACK aa929ab amitiuttarwar: ACK aa929ab theStack: ACK aa929ab glozow: ACK bitcoin@aa929ab Tree-SHA512: 17f1b11f811137497ede8851ede93fa612dc622922b5ad7ac8f065ea026d9a718db5b92325754b74d24012b4d45c4e2cd5cd439a6a8d34bbabf5da927d783970
… review fa1f3a2 doc: Clarify that squashing should happen before review (MarcoFalke) Pull request description: Unlike other repos, in our repo code review happens before merge, ideally. Thus, rebases, solving merge conflicts and squashing should happen before review, which again happens before merge. ACKs for top commit: theStack: ACK fa1f3a2 fanquake: ACK fa1f3a2 Tree-SHA512: e9222191a6e9cf9867bd1f29982526dd7b746b70dd2cc94f485256ec98ff2d3941c9b40728935e151d13795239334e334b71ad41044909cb2849f57776811a94
…buntu ea76f4a Doc: Tell howto install clang-format on Debian/Ubuntu (wodry) Pull request description: Because only macOS wasy mentioned, I was unsure if this would be a macOS specific tool. I guess Linux is more used than Mac, so Linux guide should be there, too. ACKs for top commit: hebasto: ACK ea76f4a, every system upgrade via clean installation I do the same. Tree-SHA512: 75c28540e8815cb41f4cf92784b6349978988b679e4deef9ae77ede951f93516ca13ec7b313ab72865b01273e115b49ed2b67cdcd68015af1b643a6186b190dd
1404c57 [doc] Coin: explain that IsSpent() can also mean never existed (Sjors Provoost) Pull request description: This can be especially confusing where `AccessCoin()` is used with logic like this: ```c++ while (iter.n < MAX_OUTPUTS_PER_BLOCK) { const Coin& alternate = view.AccessCoin(iter); if (!alternate.IsSpent()) return alternate; ``` ACKs for top commit: practicalswift: ACK 1404c57 MarcoFalke: ACK 1404c57 jnewbery: utACK 1404c57 Tree-SHA512: 418618dd7e08bd5cc8360e3501d0f57e34100e5101ad3b8e0a819923fa860f44c7f2fada0f8447a1af3c2601fd72bfe619b91ff2f26f7133ceaeb0c98b017b12
…from developer notes 6780a09 doc: remove obsolete `okSafeMode` RPC guideline from developer notes (Sebastian Falbesoner) Pull request description: Since the flag has been removed from the RPC command table in commit ec6902d (PR bitcoin#11179), this guideline is not relevant anymore and can be removed. ACKs for top commit: MarcoFalke: ACK 6780a09. Also, safe mode was removed completely in commit 2ae705d Tree-SHA512: 2a6b002ae302e979ce403171b79a892e32f5083792c3b0b8204edb5eb08c6b24ab77bbeeae0e3bb6d6564a6f1678cfce00eb7b5b82063b7741f89a96b0c0aef3
947f973 doc: add maxuploadtarget to bitcoin.conf example (apitko) Pull request description: picking up bitcoin#21499, author has stated they [can't squash](bitcoin#21499 (comment)). This adds the maxuploadtarget option to the `bitcoin.conf` example file. This is useful for those looking to configure their bandwidth utilization. **Changes from Original PR:** - squash commits - fix typo in commit message + reword commit message to be more appropriate - Implement review suggestions ([1](bitcoin#21499 (comment)), [2](bitcoin#21499 (comment)), [3](bitcoin#21499 (review))) - Fix spacing ACKs for top commit: jonatack: ACK 947f973 theStack: re-ACK 947f973 Tree-SHA512: 658ce6e1f204cd1c1c6f8977fd46ba1631f417d3f492e6d777b0ca49ffecc2ecfa4fbd86ca0e08c0dd5d6df4e32f0b56693e0a71bd7eafafd019e93bbcf9420f
…tions from N to N-1, because of system limitations" ea93bbe init: Fix incorrect warning "Reducing -maxconnections from N to N-1, because of system limitations" (practicalswift) Pull request description: Fix incorrect warning `Reducing -maxconnections from N to N-1, because of system limitations`. Before this patch (only the first warning is correct): ``` $ src/bitcoind -maxconnections=10000000 | grep Warning 2020-09-26T01:23:45Z Warning: Reducing -maxconnections from 10000000 to 1048417, because of system limitations. $ src/bitcoind -maxconnections=1000000 | grep Warning 2020-09-26T01:23:45Z Warning: Reducing -maxconnections from 1000000 to 999999, because of system limitations. $ src/bitcoind -maxconnections=100000 | grep Warning 2020-09-26T01:23:45Z Warning: Reducing -maxconnections from 100000 to 99999, because of system limitations. $ src/bitcoind -maxconnections=10000 | grep Warning 2020-09-26T01:23:45Z Warning: Reducing -maxconnections from 10000 to 9999, because of system limitations. $ src/bitcoind -maxconnections=1000 | grep Warning 2020-09-26T01:23:45Z Warning: Reducing -maxconnections from 1000 to 999, because of system limitations. $ src/bitcoind -maxconnections=100 | grep Warning [no warning] ``` After this patch (no incorrect warnings): ``` $ src/bitcoind -maxconnections=10000000 | grep Warning 2020-09-26T01:23:45Z Warning: Reducing -maxconnections from 10000000 to 1048417, because of system limitations. $ src/bitcoind -maxconnections=1000000 | grep Warning [no warning] $ src/bitcoind -maxconnections=100000 | grep Warning [no warning] $ src/bitcoind -maxconnections=10000 | grep Warning [no warning] $ src/bitcoind -maxconnections=1000 | grep Warning [no warning] $ src/bitcoind -maxconnections=100 | grep Warning [no warning] ``` ACKs for top commit: n-thumann: tACK bitcoin@ea93bbe, Ran on other systems running Debian 10.5 (4.19.0-8-amd64) and Debian bullseye/sid (5.3.0-1-amd64) and was able to reproduce the issue exactly as you described above on both of them. After applying your patch the issue is fixed ✌️ laanwj: Code review ACK ea93bbe theStack: tACK ea93bbe Tree-SHA512: 9b0939a1a51fdf991d11024a5d20b4f39cab1a80320b799a1d24d0250aa059666bcb1ae6dd79c941c2f2686f07f59fc0f6618b5746aa8ca6011fdd202828a930
…tcoinErrorLog 2fc5efc Update pruning tooltip, original author BitcoinErrorLog (Riccardo Spagni) Pull request description: Squashed commits from BitcoinErrorLog at his request, per the original discussion on #15: this tooltip has been adjusted to be more user-friendly and reflect what the net effect of pruning is for the user. ACKs for top commit: harding: Untested ACK 2fc5efc Sjors: utACK 2fc5efc and welcome to the dark side! jonasschnelli: ACK 2fc5efc Tree-SHA512: 45d6a7efbf4d34d20b9de439c988a39c739591b854726b6682c4cffcb23dff7d9131afab572fa0c9a8bc033c46c3878efdfbf8a984aafde632e1dfc1caa1cbbb
|
Resolved all comments @UdjinM6 fped |
PastaPastaPasta
force-pushed
the
backport-triv-pr15
branch
from
a8f3e4c
to
76b337b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.