Wrong Branch - #3769
Closed
davecgh wants to merge 64 commits into
Closed
Conversation
For at least the past 5 years, due to garbage collector design, Go has not optimized small non-pointer types to fit inside interface headers, and instead always uses a pointer to data, if the type is not already a pointer. This means that writeElement, which was written before this time to avoid unnecessary heap allocations, was now causing heap allocations for each special cased non-pointer type. This commit rewrites writeElement to only special case pointers to types, and to pass all elements by pointer. Co-authored-by: David Hill <dhill@mindcry.org>
This updates the 2.1 release branch to use the latest version of the wire module which includes reduced allocations writing messages to the p2p network. In particular, the following updated module version is used: - github.com/decred/dcrd/wire@v1.7.2
Missing checks on the SR and DC mix vector dimensions could result in failed blame assignment.
This updates the 2.1 release branch to use the latest version of the mixing module. There is no functional change for dcrd itself since the only changes to the module are for mixclient which dcrd does not use. In particular, the following updated module version is used: - github.com/decred/dcrd/mixing@v0.6.1
This allows WriteHeaderN to avoid two unnecessary allocations by using the same buffer to encode both the message header and the encoded message payload. The payload is appended to zeroed-out header bytes in the buffer, before calculating the payload checksum and serializing the header to the bytes in the beginning of the buffer. This change also results in one less Write to the network connection.
When ReadMessage/ReadMessageN error due to the message having an invalid
network magic or invalid encoding of the command string, error immediately
instead of reading the remaining byte count reported in the message header.
There is no good reason to do so, and the explanation given by the
discardInput comment is bogus.
Unlike message serialization for generic purposes (via the BtcEncode method or
the Message interface), ReadMessage{,N} is intended to only be used for
network connections which have already negotiated a protocol version. Upon
any obvious error, the socket should just be closed (as dcrd's peer and
dcrwallet's p2p package both do) without reading any more input.
The previous behavior of discarding this input to read the next wire protocol
message is not documented by ReadMessage{,N}. Changing this is considered to
be a bug fix rather than a major API break.
This special cases writes to bytes.Buffer, which is always the writer type written to by WriteMessageN. There are several optimizations that can be implemented by special casing this type: First, pulling temporary short buffers from binary freelist can be skipped entirely, and instead the binary encoding of integers can be appended directly to its existing capacity. This avoids the synchronization cost to add and remove buffers from the free list, and for applications which only ever write wire messages with WriteMessageN, the allocation and ongoing garbage collection scanning cost to for these buffers can be completely skipped. Second, special casing the buffer type in WriteVarString saves us from creating a temporary heap copy of a string, as the buffer's WriteString method can be used instead. Third, special casing the buffer allows WriteMessageN to calculate the serialize size and grow its buffer so all remaining appends for writing block and transactions will not have to reallocate the buffer's backing allocation. This same optimization can be applied to other messages in the future.
This adds two additional type cases to the optimized writes for BLAKE-256 (used by transactions and the original PoW algorithm) and BLAKE-3 (used by the current PoW algorithm). It also updates both the block header and transaction code to use these hashers during the calculation of block and transaction hashes.
A previous commit which intended to remove the no longer used *[4]byte special case (for message header checksums) from writeElement inadvertently removed this from readElement instead. Remove it from the intended place, and restore the readElement case to avoid hitting the slow path.
This adds a couple of tests to ensure a couple of the newer error codes produce the expected human-readable output that were missed when adding them. It also adds a new define to the enum to count how many there are along with a test to detect missing entries.
This modifies the code that deals with serializing and deserializing the version message int64s to be more restrictive and reject any values that result in times that require special handling for comparisons. This clamping behavior is not strictly required since code that deals with the timestamps later is careful to avoid bad timestamps in general, but it safer to just reject them at the protocol level so even if code elsewhere is not taking extra precautions there would still not be any potential issues.
This commit contains several performance and memory optimizations to improve message deserialization, both when generically called through the BtcDecode method of the Message interface and when reading wire protocol using ReadMessageN. Special cases have been added for the bytes.Buffer and bytes.Reader reader types, avoiding the allocation and synchronization costs associated with using temporary buffers from the binary freelist and avoiding the heap allocations required due to the reader interface leaking the slice parameter. ReadMessageN has reduced the number of allocations it must incur by avoiding the readElements helper function. Finally, a new internal buffer type is introduced that is used exclusively by ReadMessageN. Since the lifetime and memory of this buffer is under the control of the wire package, and the buffer is never reset or truncated, this allows transaction script deserialization to slice the internal bytes of this buffer rather than pulling temporary buffers from the scriptPool freelist and copying these into a new contiguous allocation.
The code that handles deserializing transaction scripts by way of the free list cleans up in the event of error by returning the non-nil scripts to the free list to avoid leaking them. This is making an implicit (and undocumented) assumption that it is only deserializing into empty instances and therefore any non-nil scripts in the inputs and outputs in the event of a failed deserialization came from the free list. The consequence of that is that it is possible that any slices that were set by the caller prior to a failed deserialization could incorrectly be returned to the free list and ultimately get clobbered later. While this is not an issue for dcrd since it never deserializes into non-empty instances, there is no guarantee that is true for all callers. In order to ensure safety for all callers, this nils the input and output slices prior to deserializing anything in order to ensure the aforementioned assumption is always satisfied.
This avoids needing the fragile ErrUnexpectedEOF handling and prevents read errors when the reader is buffered and next chunk of bytes is only partially available. Discovered by a failing wallet unit test which passed in a reader created by hex.NewDecoder.
This adds some additional tests for short reads. - Adds cases from some missing paths in the tests for readElement - Adds another scenario when testing readElement that reader that only provides at most one byte per Read - Adds a new test name TestShortReads that exercise each of the individual readX funcs with the various supported readers as well as a couple of readers for the default path including a one byte reader Together these tests help ensure all of the new short read paths are fully exercised.
This retracts that 1.7.3 wire release, updates the wire module copyright year in the files modified since the previous wire/v1.7.2 release and serves as a base for wire/v1.7.4.
This updates the tests which exercise various read message error paths to improve their accuracy, modernize the error detection, and make them more consistent with the newer formatting practices.
Currently, message decoding correctly reads exactly the amount of bytes that are needed and silently ignores any remaining bytes. This is correct and expected behavior, however, the entire raw buffer is also passed through to rest of the application code unaltered (aka with the extra trailing bytes) for use in some very specific cases. While there are no serious consequences to this behavior currently, it is not ideal and could potentially lead to unexpected consequences in the future. With that in mind, this adds an additional safety check to reject any messages that are not fully consumed while decoding to prevent them immediately at the protocol level rather than leaving it to code at higher layers to deal with.
This updates the 2.1 release branch to use the latest version of the wire module which includes various optimizations, test improvements, hardening against potential misuse, and a tightening of the protocol to reject messages with trailing bytes. In particular, the following updated module version is used: - github.com/decred/dcrd/wire@v1.7.5
x25519 was the ECDH used in the older client-server mixing implementation, but this was replaced with secp256k1 ECDH in the peer-to-peer implementation. Remove a lingering reference to x25519 that didn't get updated.
This adds benchmarks for signing and validating utxo proofs.
Sum(nil) creates an unnecessary heap allocation appending the digest to the nil slice. Replace these calls with direct calls to the (*blake256.Hasher256).Sum256 method to avoid unnecessary allocations and copies. While here, use the hasher's specialized Write* methods in the utxoproof package to improve readability.
Creating the hash to verify the schnorr signature can be done without causing any allocations. This has also been moved out into a new schnorrHash helper function to reuse across both signing and verification.
In addition to the existing logic which evicts orphans once an epoch has expired and when the peer that sent the orphans disconnects, this implements proactive eviction of mixing message orphans once the orphan pool grows to a maximum limit. The eviction algorithm works as follows: - Determine the source peer that sent the most remaining active orphans - Remove as many orphans sent by that peer as possible until the orphan pool size reaches 75% of the maximum allowed amount - Repeat the previous two steps as many times as necessary to reach the target pruned orphan pool size In short, it prioritizes removing the orphans sent by the peer that sent the most. This approach was chosen because it is fairly efficient and, in practice, orphan messages are quite rare after initial startup when ongoing mixing sessions are discovered, so any peer sending a lot of orphans is likely experiencing severe connectivity issues or otherwise misbehaving. It also has the added benefit of handling a variety of orphan flooding misbehavior well. A comprehensive set of tests is included to ensure the behavior works as expected.
This avoids reusing the same initial seed for multiple tests when testing with -count greater than 1. The -seed flag now requires providing the initial nonce in the string, so that the failing test can be reproduced with -count=1. If a test fails, the flag to reproduce is logged at the end of the output.
The cleanup function returned by useTestLogger must disable writes to the backend to prevent client goroutines still running after the test finishes from writing to the old *testing.T logger and panicking.
Tests run on an increased schedule by artifically ticking the epoch. This would occasionally result in a disruption tests hanging due to the two clients involved being ticked at roughly the same time but continuing with a different Unix epoch. There was a roughly 50% chance that when this happened, the test would hang, depending on which of the two clients contained the misbehaving peer. Test reliability has been improved by passing same time.Time value with the intended epoch to the testTickC channel by the test function. Client code also signals to the tests when it is waiting for a test epoch, allowing the test to wait for all clients before proceeding with the current time as the epoch.
This commit places stricter limits on message sizes without changes to wire protocol. These reduced limits help avoid memory exhaustion when mixing messages must be saved to the orphan pool.
PRs which duplicated inputs would allow for low cost entry to the mixpool and could be used to consume excessive memory resources.
This updates the 2.1 release branch to use the latest version of the mixing module which includes various optimizations, orphan source tracking, proactive orphan eviction, tighter standardness limits on mixing message sizes, and stricter rejection of malformed mixing pair requests. In particular, the following updated module version is used: - github.com/decred/dcrd/mixing@v0.7.0
This cleans up and modernizes the code related to checking transaction
inputs as follows:
- Make code more consistent with the rest of the package
- Update various errors to include more detailed information:
- Consistently include the full referenced output in errors instead of
only the transaction hash
- Use fewer abbreviations in errors to improve clarity
- Add some additional checks to assert invalid states
- Consolidate logic for adding txouts to views and improve readability
with new types and funcs
- Conform to the modern editorconfig settings
The sync manager considers the services when the initial peer is created, but the remote services are not known until the handshake completes. This adds a channel that is closed when a verack is received in order to wait for the handshake to complete prior to creating the sync manager peer to ensure the remote services are known and populated. It also moves the code that adds the peer to the server to happen after the handshake as well since it relies on the sync manager. This was discovered by changes that will be in the next commit to no longer improperly assume the remote services before they're known.
- Comments and more descriptive names for tests. - Using t.Run() provides better test metrics and removes the need to include the name of the test in log lines. - Pull a block out TestCheckAuth and make it a separate test.
This fixes an issue where the server treated an unset admin auth hash as unconditional success under the assumption that this state corresponds to the server running certificate auth. However, the server can also reach this state under basic auth by configuring limited credentials without admin credentials. In this case the limited user would have unrestricted access to all admin functions.
When both the `Origin` header and the request `Host` header lack an
explicit port (common behind reverse proxies on standard ports 443/80),
both values collapse to empty string, and `equalASCIIFold("",
"")` returns `true` - allowing **any cross-origin websocket handshake**
to succeed.
This fixes the build for 32-bit target platforms where the constant would overflow the int type.
This updates the 2.1 release branch to use the latest version of the mixing module which includes a fix for 32-bit builds. In particular, the following updated module version is used: - github.com/decred/dcrd/mixing@v0.7.1
Messages read off the wire will use a non-nil 0-length Script slice.
This updates the 2.1 release branch to use the latest version of the mixing module which includes a fix for an incorrect p2sh input script check. In particular, the following updated module version is used: - github.com/decred/dcrd/mixing@v0.7.2
This reverts commit 664d136. The changes in the peer module that required the changes in the reverted commit are not included in the release-v2.1 branch and therefore the commit should not have been backported. The commit being reverted introduced a bug unique to the release-v2.1 branch whereby the goroutines to process incoming messages are spawned before the sp.syncMgrPeer field has been set which can result in either a data race or nil dereference when the syncMgrPeer is read by the callback methods. This does not apply to the master branch since it no longer runs inbound message processing until after both the handshake has completed and the syncMgrPeer field has been set. As of reverting the offending commit, the behavior on the release-v2.1 branch now matches the same behavior in previous releases.
When peers are active in an epoch in a session that becomes abandoned to stay within the mixing limits, these peers must not be blamed for disrupting a mix if they are not lucky enough to remain in the new recreated session.
This updates the 2.1 release branch to use the latest version of the mixing module which includes an update to ensure coins that are excluded from mixes due to overall session limits of a single mix being exceeded are not greylisted. In particular, the following updated module version is used: - github.com/decred/dcrd/mixing@v0.7.3
Previously the session expiry would always be set to max uint32 due to the prs slice never being appended, thus iterating over it having no effect.
This updates the 2.1 release branch to use the latest version of the mixing module which includes a fix for a potential periodic deanonymization attack and improved session expiry. In particular, the following updated module version is used: - github.com/decred/dcrd/mixing@v0.7.4
Transactions with input values that are negative or greater than the max supply ultimately will always eventually end up invalid by checks performed much later in the validation process. Moreover, the aforementioned conditions are entirely context free. Given that, it is much more efficient and robust to simply reject any transactions that violate them as early as possible in the validation process. The context-free transaction sanity checks are the ideal location since they are among the earliest validation checks that are performed. However, unconfirmed transactions are allowed to leave the input value set to the special sentinel value of -1 (wire.NullValueIn) that signals the actual value will be filled in later. The sanity checks take place before that information is available to populate, so that case needs to be exempted and left for the later checks to reject as they already do now. With that in mind, this modifies CheckTransactionSanity to reject transactions that violate those conditions accordingly. It also adds ErrFraudAmountIn to uniquely identify when checks fail validation for that reason. Finally, it modifies the rule error conversion in the internal blockchain code to recognize and convert the new error.
This adds a few additional tests for transaction sanity checking to ensure negative values, except the special sentinel value, and values greater than the max supply are rejected as expected.
This updates the 2.1 release branch to use the latest version of the blockchain/standalone module which includes an update to reject obviously impossible values very early in the validation process. In particular, the following updated module version is used: - github.com/decred/dcrd/blockchain/standalone@v2.3.0
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Wrong Branch