Remove some of the undefined behavior from the merkle tree implementation

Describe your changes
This fixes some of the UB on the merkle tree implementation, the commit message has a few notes on the issues found (replicated below). The idea of these notes is to share some of the issues of writing unsafe code.
The final code still has some unsafe code, it was the best I could do and preserve the performance goals without breaking the existing behavior, but note there still may be issues that I'm unaware of.

This commit fixes some of the undefined behavior of the merkle tree implementation, it maintains the same behavior (as defined by the existing unit tests), and still uses some unsafe code, which should be correct.
The safety issues that I'm aware of are enlisted below:

uninit_vector did not satisfy the set_len safety requirement [1]:

The elements at old_len..new_len must be initialized.

uninit_vector breaks the initialization-invariant and causes undefined behavior for types like bool:
let v: Vec<bool> = uninit_vector(1);

As described on [2]:

Similarly, entirely uninitialized memory may have any content, while a
bool must always be true or false. Hence, creating an uninitialized bool
is undefined behavior:

uninit_vector may hit undefined behavior for types with a Drop:
let v: Vec<TypeWithDrop> = uninit_vector(1);
panic!();

As describe on the nomicon reading uninitialized memory is UB [3]:

All runtime-allocated memory in a Rust program begins its life as
uninitialized. In this state the value of the memory is an indeterminate
pile of bits that may or may not even reflect a valid state for the type
that is supposed to inhabit that location of memory. Attempting to
interpret this memory as a value of any type will cause Undefined
Behavior. Do Not Do This.

The code breaks ownership rules of the language. The memory of the merkle tree has a mutable reference with nodes at the same time it has a immutable reference with two_nodes.

As describe in the language reference:

Mutating immutable data. All data inside a const item is immutable.
Moreover, all data reached through a shared reference or data owned by
an immutable binding is immutable, unless that data is contained within
an UnsafeCell.

The memory is const cast among different types, and the language does not have data layout guarantees for these types of casts.

1- https://doc.rust-lang.org/std/vec/struct.Vec.html#method.set_len
2- https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#initialization-invariant
3- https://doc.rust-lang.org/nomicon/uninitialized.html
4- https://doc.rust-lang.org/reference/behavior-considered-undefined.html
Checklist before requesting a review

Repo forked and branch created from next according to naming convention.
Commit messages and codestyle follow conventions.
Relevant issues are linked in the PR description.
Tests added for new functionality.
Documentation/comments updated according to changes.

@hackaugusto - haven't done a review of this yet, but a quick question: how does this impact performance (if at all)?

My first try was to use only safe APIs, but I had a problem with the order of the elements. Namely the leaves are at the end of the vector.
I tried to do something like this (Snippet written from memory, probably has issues):
let mut nodes = Vec::with_capacity(n * 2);
nodes.extend(leaves.iter.rev());
let split = nodes.len();
while split > 1 {
  let bottom = nodes[nodes.len() - split..];
  let result = bottom.chunks_exact(2).map(hash).collect();
  nodes.extend(result);
  split /= 2;
}
nodes.push(ZERO);
nodes.reverse()

The problem is that the tests were failing and I didn't spend much time debugging. I also assumed the extra allocations would become a review blocker for performance reasons. So in the PR I tried to go back to unsafe code but fix the issues I could find.

@hackaugusto - haven't done a review of this yet, but a quick question: how does this impact performance (if at all)?

I didn't benchmark it, it seems we don't have any benchmarks written for the struct yet. Should I add some?

Ah! There were some benchmarks for this in Winterfell - but we probably didn't carry them over.
Yes, would be good to add benchmarks to get an idea of before/after performance.

It seems to me that if we allocate, then it's no longer undefined behavior because the memory is a well-formed, contiguous sequence of bytes (that needs to be overwritten by something). If we try to read an element before initialization, then it's definitely UB, but then we are also violating the contract of uninit_vector. Wdyt?

That was my original reasoning as well. If you allocated a vector with uninint_vector() you are responsible for making sure no values are used before they are initialized, and that's why it is marked unsafe.
I agree that maybe some extra checks would be good to make sure that length is not zero and maybe something else.

It seems to me that if we allocate, then it's no longer undefined behavior because the memory is a well-formed, contiguous sequence of bytes (that needs to be overwritten by something). If we try to read an element before initialization, then it's definitely UB, but then we are also violating the contract of uninit_vector. Wdyt?

Please read the references I added to the commit. Assigning invalid values is UB, reading is not necessary for it to be UB:

Producing an invalid value, even in private fields and locals. "Producing" a value happens any time a value is assigned to or read from a place, passed to a function/primitive operation or returned from a function/primitive operation. The following values are invalid (at their respective type):

From the language specification

Please read the references I added to the commit. Assigning invalid values is UB, reading is not necessary for it to be UB

That would happen only if the user fails to initialize the bool properly. Some architectures will optimize things for boolean types, and this could might be UB:
let foo: Vec<bool> = unsafe { uninit_vector(50) };
foo[0] = true;

We need to keep in mind that this is again consequence of the user not initializing the values properly, and also that bool is a rather special case that is not applicable to u64. I think the original contract of the function (that the user, somehow, needs to initialize the values properly) is still valid.
But I get your point, he will cause UB outside the unsafe block, and that is not a good thing. What if, instead, we go like this:
use core::mem::MaybeUninit;

pub unsafe fn uininit_alloc<T>(length: usize) -> MaybeUninit<Vec<T>> {
    let mut vector = Vec::with_capacity(length);
    vector.set_len(length);
    MaybeUninit::new(vector)
}

This way, we will incentivize (not force tho, he can still cast pointers) the user to initialize his values entirely inside the unsafe block. When he wants to get the vec, he will have to call unsafe MaybeUninit::assume_init, leaving him no option get his vec via safe API.
If we go that way, we end adding up just like 2 or 3 assembly instructions, so that is almost negligible as we will not have to alloc in any way. Wdyt?

As for the second point, the funny thing is I spotted a problem there today too, but with a different root:
https://github.com/0xPolygonMiden/crypto/pull/54#discussion_r1106927457
That one is mostly a matter of guidelines. We have a logical guarantee that the values will not overlap, but then ofc the program can contain a bug or something like that. I wouldn't say we are violating memory safety, I would say we are violating a community guideline.

That would happen only if the user fails to initialize the bool properly. Some architectures will optimize things for boolean types, and this could be UB:

I don't agree, uninit_vector is asserting the initialization was done when it calls set_len. This is part of that method's safety requirement:

The elements at old_len..new_len must be initialized.

The initialization is no longer in the user, it can't be because uninit_vector didn't return a MaybeUnit (AFAIK the this is the only wrapper that can be used to declare uninitialized data).

If we go that way, we end adding up just like 2 or 3 assembly instructions, so that is almost negligible as we will not have to alloc in any way. Wdyt?

I think this API is still broken because of the point above. I think instead of adding an API we should just use the std library spare_capacity_mut (this is what I did in this patch)

That one is mostly a matter of guidelines. We have a logical guarantee that the values will not overlap, but then ofc the program can contain a bug or something like that. I wouldn't say we are violating memory safety, I would say we are violating a community guideline.

I don't agree, I believe that code is incorrect. The language specification is very clear on aliasing rules, there must never be simultaneously a writer and a reader for the same memory.  This is the whole point of the borrow checker. The compiler is allowed to cache reads from the path it is said to be immutable, and never refresh it with the new data.

I think this API is still broken because of the point above. I think instead of adding an API we should just use the std library spare_capacity_mut (this is what I did in this patch)

That sounds fair. There is a trade off here:

If we go fn<T>(usize) -> MaybeUninit<Vec<T>>, we have O(1) performance overhead (i.e. once to instantiate, and again to assume_init).

If we go spare_capacity_mut, we have O(n) performance overhead (i.e. for every iteration when we are initializing the elements), but we have better API guarantees.

I don't agree, I believe that code is incorrect. The language specification is very clear on this particular point of the aliasing rules, there must never be simultaneously a writer and a reader for the same memory. This is the whole point of the borrow checker. The compiler is allowed to cache reads from the path it is said to be immutable, and never refresh it with the new data. That code can totally end up hashing the data coming from the uninitialized path because it is reading through the immutable path, while writing the result to another piece of memory.
Here is a reference: https://doc.rust-lang.org/nomicon/aliasing.html

The trick of that code is that the read area will never overlap with the write area, that's why it works. The same result could be achieved with split_at_mut tho, but with a small overhead.

The trick of that code is that the read area will never overlap with the write area, that's why it works. The same result could be achieved with split_at_mut tho, but with a small overhead.

I don't think this is correct. The memory does overlap, I guess you're thinking in term of memory addresses per expression. This is not sufficient. One has to think the provenance of the whole allocation. Here is a reference:

Aliasing occurs when one pointer or reference points to a "span" of memory that overlaps with the span of another pointer or reference. A span of memory is similar to how a slice works: there's a base byte address as well as a length in bytes.

https://rust-lang.github.io/unsafe-code-guidelines/glossary.html#aliasing

If we go spare_capacity_mut, we have O(n) performance overhead (i.e. for every iteration when we are initializing the elements), but we have better API guarantees.

I don't understand where the O(n) is coming from. It should have the same performance and be safer.

I don't think this is correct. The memory does overlap, I guess you're thinking in term of memory addresses per expression. This is not sufficient. One has to think the provenance of the whole allocation. Here is a reference:

I think we are deviating a bit from the topic. That provenance example is applicable for when you create pointers out of thin air, but in this case the pointers are already sound from the beginning of the thread (i.e. self.nodes exists), and we just split the reads/writes while this instance is available and "pinned". That's why, again, using split_at_mut would produce the same effect (with higher overhead).

Edit: Another thing, the const cast can only be done back and forth to the same data type, not to different data types, the data layout is not guaranteed to be the same, so that is UB too.

True, but the memory layout is guaranteed to be the same: they are both Words. I added a small check here, using that same PR, just to assert that if it ever changes (i.e. RpoDigest changes to something else than a Word - highly unlikely, but possible), the CI will let us know
https://github.com/0xPolygonMiden/crypto/blob/e3c92ac5b8e846e3b6ed51eae27cd4ebe901ddc7/src/merkle/merkle_tree.rs#L272-L298

I don't understand where the O(n) is coming from. It should have the same performance and be safer.

It's about the point made here: https://github.com/0xPolygonMiden/crypto/pull/58#issuecomment-1432052020
As described, the performance will not be the same. In this example, the overhead will occur only once (when we call uininit_alloc). But if we use spare_capacity_mut, that very same overhead will happen n times (as they will be bound to each element of the slice, instead to the vec itself).

I think we should take a step back here to see what we are trying to achieve. From my side, I'm looking at the following:

I'd like to have the code be as performant as possible.
I'd like the code to be as readable as possible.
I'd like the code to be as correct as possible.

I think the original code more or less meets the first two goals and is not technically correct (in Rust's definition of this word). And in terms of correctness, it seems to me that there are two issues:

uninit_vector() returns Vec<T> and while the contract of the method states that T is not initialized, the language/compiler has no way of knowing that.
We manually create two references to the same data - one mutable and another one immutable and use both outside of unsafe code, and this violates the rules of the language.

Regarding the first one, I'd say that we actually do want to get access to a vector of uninitialized memory (that's the whole point of the method). We could use spare_capacity_mut() method - but I wonder if we could just return Vec<MaybeUninit<T>> from uninit_vector().
Regarding the second one, we know that the code is logically correct and can result in incorrect behavior only if the compiler makes some assumptions about the underlying data. So, could we just mark this whole segment as unsafe or is there some other way to tell the compiler not to make assumptions about the data?

Regarding the first one, I'd say that we actually do want to get access to a vector of uninitialized memory (that's the whole point of the method). We could use spare_capacity_mut() method - but I wonder if we could just return Vec<MaybeUninit<T>> from uninit_vector().

Just a nit: did you mean MaybeUninit<Vec<T>>? If yes, then IMO it's safe & not always sound (as the user might use some heap types as T, and that would be UB). Alternatively, if we really want to make the API 100% sound, we can MaybeUninit<Vec<Word>> or introduce some constraints such as T: Copy.
However, if Vec<MaybeUninit<T>>, then I think it's better to go solely with spare_capacity_mut as the stdlib will already do what we are trying to implement.

Regarding the second one, we know that the code is logically correct and can result in incorrect behavior only if the compiler makes some assumptions about the underlying data. So, could we just mark this whole segment as unsafe or is there some other way to tell the compiler not to make assumptions about the data?

I don't think that will be an improvement because the unsafe will be a flag for the users. The compiler will just ignore it and try to optimize. The concern is precisely when that happens: when the compiler optimizes stuff, it will move pointers here and there based on their mutability, lifetimes, and a bunch of other very, very complicated stuff. It's reasonable to say that is unpredictable.
The question is how to stop the compiler from doing that for this case, and unsafe won't do the trick.

However, if Vec<MaybeUninit>, then I think it's better to go solely with spare_capacity_mut as the stdlib will already do what we are trying to implement.

I'm biased towards this option too, it is a two liner:
let mut v = Vec::with_capacity(n);
let mut b = v.spare_capacity_mut();

It seems MaybeUninit doesn't have utilities that are helpful to work with Vecs. I guess we can do this: (??)
fn uninit_vector<T>(size: usize) -> Vec<MaybeUninit<T>> {
    let mut v: Vec<MaybeUninit<T>> = Vec::with_capacity(size);
    // is this safe?!
    unsafe { v.set_len(size); }
    v
}

Regarding the second one, we know that the code is logically correct and can result in incorrect behavior

I'm being picky here, but the code is not logically correct. By definition code that runs into UB is unsound:

Accordingly, we say that a library (or an individual function) is sound if it is impossible for safe code to cause Undefined Behavior using its public API. Conversely, the library/function is unsound if safe code can cause Undefined Behavior.

definition

only if the compiler makes some assumptions about the underlying data.

I would assume the compiler is allowed to do this already. Imagine the following code:
fn build_tree(els: Vec<Word>)
  // **inlining may be prevented** because this is coming from a different crate
  let nodes1 = uninit_vector(els.len() * 2);
  nodes1[..2].copy_from_slice(&els);

  // this is a const* the compiler is allowed to assume these values do not change
  let two_nodes = unsafe {
     slice::from_raw_parts(nodes.as_ptr() as *const [RpoDigest; 2], n)
  };

  // here the values are modified, from the compiler perspective this must be a different
  // location than the read above
  for i in (1..n).rev() {
    nodes1[i] = Rpo256::merge(&two_nodes[i]).into();
  }

  // give ownership to caller
  nodes1
}

fn tree() {
  let els = ...;
  let nodes2 = Vec::with_capacity(els.len() * 2);

  // this code can be inlined, the compiler can reason that:
  // 1 `nodes1` and `nodes2` are different allocs (because uninit_vector is not inlined)
  // 2. `two_nodes` is from const, so it can just do loads from `nodes1`
  // 3. write goes to a temporary `nodes1`, which are copied to `nodes2`, so the compiler can just forwards writes to `nodes2`
  // the above is a completely valid reasoning on the part of the compiler, but means the loop
  // reads uninitialized data, and the second layer onwards is incorrect
  nodes2.extend_from_slice(&build_tree(els));
}

And if it doesn't do that already, it is probably a question of when. AFAIK the only interface to have a reader + writer to the same data is through interior mutability, which is done with an UnsafeCell.

So, could we just mark this whole segment as unsafe or is there some other way to tell the compiler not to make assumptions about the data?

Wrapping code with UB with unsafe does not fix it, it is still UB, from the language reference:

Rust code is incorrect if it exhibits any of the behaviors in the following list. This includes code within unsafe blocks and unsafe functions. unsafe only means that avoiding undefined behavior is on the programmer; it does not change anything about the fact that Rust programs must never cause undefined behavior.

And later:

Mutating immutable data. All data inside a const item is immutable. Moreover, all data reached through a shared reference or data owned by an immutable binding is immutable, unless that data is contained within an UnsafeCell.

So AFAIU the only way to fix the issue, is to not alias the memory using split_at_mut

I updated the code to have the benchmarking code, I will run them tomorrow.
MerkleTree/construction/131072
                        time:   [636.81 ms 637.42 ms 638.03 ms]
                        change: [+0.5970% +0.8149% +1.0297%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) low mild
MerkleTree/construction/262144
                        time:   [1.2743 s 1.2755 s 1.2767 s]
                        change: [+0.8987% +1.0568% +1.2249%] (p = 0.00 < 0.05)
                        Change within noise threshold.

The benchmark was executed with:
cargo clean

git checkout eda391c5f41157ecddcb779c974c9deebdac618c
caffeinate -i cargo bench --bench merkle_tree -- --save-baseline ./baseline --measurement-time 200

git checkout 18ddb2836ba0c6a8508611f1e3d8b7d3123bfa69
caffeinate -i cargo bench --bench merkle_tree -- --baseline ./baseline --measurement-time 200

65536:

131072:

262144:

Will cause a small O(n) overhead in order to assumit_init

assume_init is type level only, there is no overhead.

To give further context, if you do something life
pub fn create(v: Vec<MaybeUninit<i32>>) -> Vec<i32> {
    v.into_iter().map(|e| unsafe { e.assume_init() }).collect()
}

Then indeed it will be a zero cost abstraction. But such simple case, most likely, won't exist on the real world.
The added overhead is usually to satisfy the constraints imposed by the additional layer of safety (like memcpy restrictions, etc). If we find equivalence to that, then we are definitely on the good spot!

I ran the merkle_tree tests over miri, it did caught the UB from the memory overlap on the existing code:
test merkle::merkle_tree::tests::build_merkle_tree ... error: Undefined Behavior: trying to retag from <374206> for SharedReadOnly permission at alloc145893[0x80], but that tag does not exist in the borrow stack for this location
   --> src/merkle/merkle_tree.rs:45:38
    |
45  |             nodes[i] = Rpo256::merge(&pairs[i]).into();
    |                                      ^^^^^^^^^
    |                                      |
    |                                      trying to retag from <374206> for SharedReadOnly permission at alloc145893[0x80], but that tag does not exist in the borrow stack for this location
    |                                      this error occurs as part of retag at alloc145893[0x80..0xc0]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information

That problem is fixed on this branch, test with:
cargo miri test -- merkle::merkle_tree::tests::build_merkle_tree

That doesn't mean there are no errors in the code though. Additionally, the other tests caught the same bug on the update_leaf method:
https://github.com/0xPolygonMiden/crypto/blob/398af59045efe16181140475742c7535618a9c75/src/merkle/merkle_tree.rs#L126-L127
I can work on the fix once we agree on things. As just to re-iterate, if we have unsafe code, I really think we should run miri in the CI

I think we need to focus on the facts in order to make that decision. Miri will behave more like a linter than a memory inspector when performing that analysis. We can use, for example, valgrind. Given the following code:
use miden_crypto::{merkle::MerkleTree, Felt, Word};
use std::{fs::File, io::Read};

fn gen_words(count: usize) -> Vec<Word> {
    let mut buffer = vec![0u8; count * 8 * 4];
    let mut bytes = File::open("/dev/random").unwrap();
    bytes.read_exact(&mut buffer).unwrap();
    buffer
        .chunks_exact(8 * 4)
        .map(|chunk| {
            let a = <[u8; 8]>::try_from(&chunk[..8]).unwrap();
            let b = <[u8; 8]>::try_from(&chunk[..8]).unwrap();
            let c = <[u8; 8]>::try_from(&chunk[..8]).unwrap();
            let d = <[u8; 8]>::try_from(&chunk[..8]).unwrap();

            let a = u64::from_le_bytes(a);
            let b = u64::from_le_bytes(b);
            let c = u64::from_le_bytes(c);
            let d = u64::from_le_bytes(d);

            let a = Felt::new(a);
            let b = Felt::new(b);
            let c = Felt::new(c);
            let d = Felt::new(d);

            [a, b, c, d]
        })
        .collect()
}

fn main() {
    let leaves = gen_words(32);
    let mut tree = MerkleTree::new(leaves).unwrap();

    gen_words(32).into_iter().enumerate().for_each(|(i, w)| {
        tree.update_leaf(i as u64, w).unwrap();
    });

    println!("{:x?}", tree.root()[0].inner());
}

We can run a full suite of valgrind, and no read over initialize, overlapping memcpy, or any other memory error will happen.
⚡ valgrind --leak-check=full --undef-value-errors=yes --show-mismatched-frees=yes --track-origins=yes --show-reachable=yes ./target/debug/miden-crypto
==189155== Memcheck, a memory error detector
==189155== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==189155== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==189155== Command: ./target/debug/miden-crypto
==189155==
f763ca6f6ba2663
==189155==
==189155== HEAP SUMMARY:
==189155==     in use at exit: 0 bytes in 0 blocks
==189155==   total heap usage: 16 allocs, 16 frees, 9,325 bytes allocated
==189155==
==189155== All heap blocks were freed -- no leaks are possible
==189155==
==189155== For lists of detected and suppressed errors, rerun with: -s
==189155== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

We don't have a memory error. What we have is API that is not according to the community guidelines, so IMO we should focus on that.

We can adapt it to something that miri can execute in isolation, and have the same result. Miri will report a lint, valgrind will report no memory errors
use miden_crypto::{merkle::MerkleTree, Felt, Word};

fn gen_words(mut seed: u64, count: usize) -> Vec<Word> {
    let mut leaves = Vec::with_capacity(count);
    for _ in 0..count {
        let a = Felt::new(seed);
        let b = Felt::new(seed + 1);
        let c = Felt::new(seed + 2);
        let d = Felt::new(seed + 3);
        seed += 4;

        leaves.push([a, b, c, d]);
    }
    leaves
}

fn main() {
    let leaves = gen_words(1_000, 32);
    let mut tree = MerkleTree::new(leaves).unwrap();

    gen_words(1_000_000, 32)
        .into_iter()
        .enumerate()
        .for_each(|(i, w)| {
            tree.update_leaf(i as u64, w).unwrap();
        });

    println!("{:x?}", tree.root()[0].inner());
}

Miri will behave more like a linter than a memory inspector when performing that analysis. We can use, for example, valgrind.

I'm not sure I understand what you mean, miri and valgrind are different tools, and I'm in favor of using valgrind too and any other sanitizer for that matter.
But it sounds you're claiming because valgrind didn't detect the same UB reported by miri, then miri report is invalid, and I don't think that is correct. memcheck can detect quite a few memory bugs, but AFAIK it can't understand Rust borrowing rules, so it can possibly detect the same UB.
As a note, miri is not a linter, miri is an interpreter:

An experimental interpreter for Rust's mid-level intermediate representation (MIR). It can run binaries and test suites of cargo projects and detect certain classes of undefined behavior, for example [...] Experimental: Violations of the Stacked Borrows rules governing aliasing for reference types

https://github.com/rust-lang/miri

We don't have a memory error. What we have is API that is not according to the community guidelines, so IMO we should focus on that.

could you expand on that? we seem to be disagreeing on this point. I think we are disagreeing on the definition of what we have (i.e. aliasing or not), and what are the results of that (UB or not).

could you expand on that? we seem to be disagreeing on this point. I think we are disagreeing on the definition of what we have (i.e. aliasing or not), and what are the results of that (UB or not).

We don't have aliasing nor UB.
Aliasing
What we do now is:

Take a pair of words from some position
Use this pair as input of the RPO permutation, generating a new word
Move this new word to a lower position
Repeat until we move the computed word to the position 0

Therefore, aliasing is impossible there.
The only reason we need unsafe code there is because #59 isn't solved. If we have Word === RpoDigest, then we don't even need unsafe code as we could have just a triplet of indexes, and move them around:

Reference a pair (i, j) and a relative root k
Pass the words referenced by the pairs as argument of rpo hash
Replace the word indexed by k with the result of the hash
i << 1, j << 1, k << 1 until k == 0

UB
As discussed, we would have UB only if we had a Vec<bool>, Vec<String>, etc; constructed via the uninit vec function. However, since we have Vec<Word>, UB will not happen.

can you also clarify the point about miri? is the claim that miri report is not reliable? could you clarify why?

We might be entering the domain of personal opinions here. I'm not very fond of Miri precisely because it mixes different subjects and attempts to be a sort of panacea, checking both Rust rules and memory operations. I believe their claims and goals are way too bold. But I don't think this should be of relevance for this discussion as it is highly subjective. We have people who loves Miri, and we have people who doesn't like it that much. I happen to belong to the second group :D
In that particular case, Miri is checking for a Rust rule, not a memory violation. It is clear that, in Rust, we cannot have mutable & immutable refs that overlaps, and this is precisely what Miri checks there and points as candidate UB - without stating it is effectively a memory violation (when it is not, as we saw with valgrind). This rule is closer to a language rule than to a memory correctness rule. That's why I think it's more of a lint, and it gets blurry as we don't know what exactly to expect from Miri in terms of report (because of the aforementioned loosely defined scope of checks & rules).

Okay, this is what I meant before. You're claiming that m[0] and m[1] do not alias, because they have different pointer values. However, my understand is that they do alias, because the pointers are based on the same allocation.

Instead of documentation, we can describe in details what is happening.
Let's take as example a single iteration of the routine above.

Let the size of a word be w.
Let the address of self.nodes[0] be a.
Let the address of the last element of self.nodes be a + l.
Let the address of the first element that will be used as merged input be a + b.
Let the address of the merge target be a + c.

What we know:

b > c
Rpo256::merge will allocate a new word, safely, that will not overlap in any way with [a..a + l + w]

What happens there is we call Rpo256::merge([a + b..a + b + 2 * w]) and generate a new word at m. There is no memory error as Rpo256 will receive a perfectly valid slice as input.
After Rpo256::merge finishes allocating the new word at m, only then we _[a + c] := m. It is not like we are using the contents in between [a..a + l + w] to set _[a + c], but we instead use the new valid m.
Aliasing (as a problem) could happen if we tried to do something like, given some function f, _[a + c] := f(_[a + b..a + b + 2 * w]), but then, we have an entirely new address m.

My understanding is there are other sources of UB, for example as far as I understand, the const cast among different types is UB. Could you point me to documentation that clarify the casting rules between different types?

That implementation happens to be safe because we have the guarantee that Word and RpoDigest have the same layout.
We can use the documentation of transmute as reference. Here they state that "if both types are valid for the representation", then we are good. And that is true for this conversion, as they are not only two valid states, but actually the exact same state/representation (both are words with different names).
IMO, we should solve #59 and not use unsafe at all in this function as what we are doing there is so simple that shouldn't require any fancy solution. I think that would be the best course of action to solve this part of the problem. Regarding the uninit_vec, the options I suggest are the ones enumerated here.

Thanks for taking the time with this discussion :)

No problem! As mentioned above, these discussions are always very healthy in the sense of trying to disclose overlooked problems that can be critical.

this is precisely what Miri checks there and points as candidate UB

I'm asking if you consider the report unreliable because the tool does report UB, and at the same time you're saying there is none, which seems contradictory to me. I don't understand why you claim that miri reports a candidate of UB, to me it is reporting an occurrence of UB, that is, unless you consider that a false-positive, and then it would be nice to understand why.

In that particular case, Miri is checking for a Rust rule, not a memory violation

Memory violation and undefined behavior are not the same thing. For example compilers are allowed to remove code with UB example. That means UB doesn't have to manifest as memory violation, as a matter of fact in the case linked there won't even be any code to manifest any runtime behavior! What I'm trying to say is that valgrind != miri, they do operate on different levels, and the lack of a report on one does not mean there are no issues or invalidate the other.

Instead of documentation, we can describe in details what is happening.

Just to clarify, I'm not asking for you to write documentation, but instead to use an existent reference of the language, the reason is that  we are considering different aliasing rules, and I want to understand the one you're basing your rationale on. It is better to use a reference (as much as possible since aliasing rules are not fully spec'ed out in Rust), so that we can solve the differences in understanding.
The aliasing rules I'm trying to follow are defined the Rust language specification here. Which in turn are defined by llvm here (also stacked borrows). The way I interpret that definition boils down to every pointer that overlaps is aliasing. With this interpretation the pointers nodes and two_nodes are aliased. And the language explicitly forbids simultaneous use of a write and read aliased pointers (the first being nodes, the later being two_nodes). Reference:

If you have a reference &T, then normally in Rust the compiler performs optimizations based on the knowledge that &T points to immutable data. Mutating that data, for example through an alias or by transmuting an &T into an &mut T, is considered undefined behavior.

We can use the documentation of transmute as reference. Here they state that "if both types are valid for the representation", then we are good.

I went over the documentation, I couldn't find the alignment of the u64, but it seems that the 1-tuple is transparent. I wonder if the logic of transmute translate to *const casting.

I think we should take a step back and return the focus to the present issue as we are going too far away from the scope. We can start another discussion on whether or not we should integrate miri into the CI, but I don't think it is relevant to the solution of the present PR.
To recap; the problems we have are:

uninit_vector might cause UB if T isn't picked correctly

As proposal, we probably should refactor uninit_vector, either restricting it to a Vec<Word> or some invariant of MaybeUninit

unsafe cast of &[RpoDigest] as &[Word]

We can get rid of this entirely without any unsafe code if we declare Word as a struct and use it as output of Rpo256, and update merge to take a slice instead of an array. Anything else, IMO, will be overkill, considering the simplicity of updating a Merkle path in a single Vec. So I suggest against the presented ad-hoc changes & go for the path that is the long term solution for the issue.
I suggest we take the remainder topics in isolation & dedicated discussions that could possibly result in some concrete action to improve our general workflow.

I think we should take a step back and return the focus to the present issue as we are going too far away from the scope. We can start another discussion on whether or not we should integrate miri into the CI, but I don't think it is relevant to the solution of the present PR.

It seems you're missing the point of what I'm trying to say. I'm going try to state it:

Undefined Behavior is unsound, code with UB is incorrect
The language defines that at any point in time, either there can be one active writer, or multiple active readers. Never both at the same time
the code here breaks that requirement. nodes is a writer, pairs is a reader, they are aliased to the same memory region. Therefore it is UB and incorrect.

Miri is just the tool used to verify the above

* the code [here](https://github.com/0xPolygonMiden/crypto/blob/main/src/merkle/merkle_tree.rs#L45) breaks that requirement. `nodes` is a writer, `pairs` is a reader, they are aliased to the same memory region. Therefore it is UB and incorrect.

Here is one example:
// `merged` is allocated outside the aliasing space of `nodes`
let nodes_immutable = &nodes[48..50];
let merged = Rpo256::merge(nodes_immutable);

// then, we move `merged` to nodes
let nodes_mutable = &mut nodes[24];
*nodes_mutable = merged;

// this would be valid even like this
nodes[24] = Rpo256::merge(&nodes[48..50]);

// or like this
*(&mut nodes[24]) = Rpo256::merge(&nodes[48..50]);

miri mixes up there because he can't understand that pairs is actually nodes as it was a cast pointer.

Your snippet of code is not the same, the issue is the loop. Here is the original snippet:
for i in (1..n).rev() {
  nodes[i] = Rpo256::merge(&pairs[i]).into();
}

nodes and pairs reference the same underlying region of memory (pairs is created from nodes), this means the two alias. nodes is a write reference, pairs is a read reference, this means that once nodes is used, the reference of pairs is invalidated. That means that pairs can not be used together with nodes is a loop.

But now it should be clear how this is a language constraint and not an aliasing problem as merged doesn't live inside nodes.
Wdyt if we go for the solution I proposed? (to use RpoDigest as Word so we don't even need to cast the pointer in the first place)

But now it should be clear how this is a language constraint and not an aliasing problem as merged doesn't live inside nodes.

I really don't understand what you are trying to say with this. Of course UB is a language constraint, it is defined as unsound and therefore incorrect.
merged is unrelated to what I'm walking about which is having a writer and a reader existing at the same time for aliased memory. merged could also be data coming from a generator. The issue is that reading and write to the same memory is UB.

Wdyt if we go for the solution I proposed? (to use RpoDigest as Word so we don't even need to cast the pointer in the first place)

I think this is a good solution for the casting 👍

This bizarre snippet, for example, also passes with miri

That is because the lifetime of &nodes[i * 2..i * 2 + 2] doesn't have to outlive the loop like pairs does.

The problem with Rpo256::merge is the expected argument: &[RpoDigest; 2].
In this case, we will have a &[Word], so we will be forced to cast the slice again, unless we update Rpo256::merge to be &[RpoDigest]
The final code would look something like this
while i > 0 {
    nodes[i] = Rpo256::merge(&nodes[2 * i..2 * (i + 1)]);
    i >>= 1;
}

That is because the lifetime of &nodes[i * 2..i * 2 + 2] doesn't have to outlive the loop like pairs does.
edit: I haven't thought much about the left hand side though, was that to satisfy the borrow checker?

Oh no, that was just to make the expression even weirder xD
We can also make a pointer outside the loop and ignore the borrow rules completely
this one is ok with miri
let p = (&mut nodes[0..]).as_mut_ptr();
for i in 0..12 {
    *(unsafe { p.add(i).as_mut().unwrap() }) = Rpo256::merge(&nodes[i * 2..i * 2 + 2]);
}

or worse (but this one fails on miri)
let a = (&nodes[0..]).as_ptr();
let b = a as *mut u8;
for i in 0..12 {
    let x = unsafe { b.add(i).as_mut().unwrap() };
    // when we perform this operation, miri can't keep track of the origins of `a` as it is a cast
    // pointer, but in practice we are doing something as trivial as `nodes[i] =
    // `Rpo256::merge(&nodes[i * 2..i * 2 + 2])`
    let y = unsafe { core::slice::from_raw_parts(a.add(i * 2), 2) };
    *x = Rpo256::merge(y);
}

I haven't tested the code you pasted, but reading from it, miri's behavior seems totally fine

this one is ok with miri

Yes, because the read pointer doesn't have to outlive the loop, it's lifetime is only the call to Rpo256::merge

but this one fails on miri

yes, because a is a read pointer that outlives the loop, and x a writer to the aliased memory used inside the loop that invalidates it. Once again, it is not allowed to have a read and writer alive at the same time!!! This is super important for the language!

miri can't keep track of the origins of a

are you saying this because of miri's error message? but that tag does not exist in the borrow stack for this location. The lack of a tag is not miri losing track of the memory, it is how the undefined behavior is detect! please read the stacked borrow paper

Let's try to not walk in circles around this. I think we sufficiently covered all the points.
The idea of these snippets is to demonstrate precisely how the progression from a perfectly valid & safe code, when expressed differently raises errors with Miri (i.e. the trick I did to confuse the tag of the address, having in mind precisely how miri works behind the curtains & how it ended up reporting the Merkle tree constructor error).
I don't think it will be profitable for this PR to engage on a discussion if this is a flaw of the original paper, miri, or any other tooling. As I mentioned above, I don't think miri is a suitable tool for general purpose checks as it is overly defensive on some occasions (such as this one). It means we should always dig whenever it reports an error, and not take that as fundamental cause to discard some code, or sacrifice performance.
Turns out this case is completely trivial to be replaced by 100% safe code with some API adjustments here & there.

@vlopes11 the goal was not to go in circles, but to agree on very subtle ideas. I assumed you could know something I didn't, and I hoped you would take the time to backup your claims such as I don't think miri is a suitable tool for general purpose checks as it is overly defensive on some occasions (such as this one) with references so that I could update my mental model. Sentences like these spread through out the discussion implies you think the previous two iterations of the code doesn't alias and doesn't have UB, which to me it is clearly incorrect, but you never provided any references to backup your claims.
Anyways, the simple loop does fix the issue, I will try to update the PR with that approach in case it makes easier to get this through.

I updated the code with the simpler loop. This is the baseline:
MerkleTree/construction/65536
                        time:   [313.33 ms 313.81 ms 314.32 ms]
MerkleTree/construction/131072
                        time:   [636.53 ms 636.96 ms 637.36 ms]
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) low mild
MerkleTree/construction/262144
                        time:   [1.2694 s 1.2709 s 1.2723 s]

This is the code with a simple loop:
MerkleTree/construction/65536
                        time:   [313.61 ms 313.87 ms 314.16 ms]
                        change: [-0.1629% +0.0180% +0.2015%] (p = 0.85 > 0.05)
                        No change in performance detected.
Found 16 outliers among 100 measurements (16.00%)
  11 (11.00%) high mild
  5 (5.00%) high severe
MerkleTree/construction/131072
                        time:   [635.06 ms 635.91 ms 636.75 ms]
                        change: [-0.3256% -0.1633% -0.0174%] (p = 0.03 < 0.05)
                        Change within noise threshold.
MerkleTree/construction/262144
                        time:   [1.2825 s 1.2831 s 1.2837 s]
                        change: [+0.8370% +0.9581% +1.0873%] (p = 0.00 < 0.05)
                        Change within noise threshold.

65536:

131072:

262144:

I also did a second try using pos * 2 on every iteration instead of 2 variables. These are the benchmarks:
MerkleTree/construction/65536
                        time:   [315.64 ms 316.16 ms 316.68 ms]
                        change: [+0.5071% +0.7461% +0.9618%] (p = 0.00 < 0.05)
                        Change within noise threshold.
MerkleTree/construction/131072
                        time:   [641.53 ms 641.77 ms 642.01 ms]
                        change: [+0.6802% +0.7562% +0.8324%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) low mild
  1 (1.00%) high mild
MerkleTree/construction/262144
                        time:   [1.2835 s 1.2839 s 1.2843 s]
                        change: [+0.9037% +1.0202% +1.1411%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) low mild
  1 (1.00%) high mild

65536:

131072:

262144:

based on the results above I kept the version with two variables.

@hackaugusto there is probably a misunderstanding. Even tho I mentioned it a couple of times, I never intended to discuss Miri details within the scope of this PR. What I suggested is we have a dedicated call and/or topic of discussion if we want to explore the design of Miri and how it can help us to evaluate the decision making regarding the safety of our code.
Back to the PR: did you manage to do it without changing the interface of Rpo256?

Back to the PR: did you manage to do it without changing the interface of Rpo256?

The PR is updated. I used let hash = Rpo256::hash_elements(&[left, right].concat()).into(); as a work around. Changing the API would make code clearer, but at least ATM doesn't seem necessary.

Hey @hackaugusto @vlopes11 there's a lot of discussion which I haven't fully caught up on - it seems like maybe you've reached some consensus, but there are still a few things to discuss and finalize. (But please correct me if I'm wrong) Do you think we should pause the discussion here and finish it in TG or on a call, then come back and update with the conclusion & decisions?

@grjte I think it is a good idea to have a call just to make sure we are all on the same page.
Here is my current summary of the issues:

uninit_vector doesn't restrict T to be Copy or use MaybeUninit
nodes and pairs alias

We have two options to fix 1:

Use MaybeUninit: unsafe fn uninit_vector<T>(size: usize) -> Vec<MaybeUninit<T>>
Require Copy: unsafe fn uninit_vector<T + Copy>(size: usize) -> Vec<MaybeUninit<T>>
We haven't agreed on the return type, my understanding is that @vlopes11 prefers Vec<T>

I'm not sure if @vlopes11 agrees on point 2

Update: During our sync-up today it was agreed that this should be fixed on winterfell first.


MINOR: apply default.api.timeout.ms to fetchEndOffsets

Fixes the admin client timeout supplied to future.get(timeout) to always time out based on the configured default.api.timeout.ms

Test this please

It looks like all the review comments are addressed, and all the tests passed, so I'll proceed to merge.

Oh, actually @ableegoldman , it looks like there was a conflict with the other PR I just merged.

I just resolved the conflicts.

Test this please

Ok to test

Retest this please

... or not

Test this please

Test this please

Test this please

Retest this please

Test this please

\o/

Test this please

Test this please

Checkstyle error:
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk8-scala2.12/streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamsPartitionAssignorTest.java:21:8: Unused import - org.apache.kafka.clients.admin.AdminClientConfig. [UnusedImports]

Retest this please.

Retest this please.

Merged to trunk and cherry-picked to 2.6. Do we want this in 2.5, too? If yes, we would need a new PR. Not possible to cherry-pick.

Thanks! I actually don't think there ended up being anything relevant t o2.5 in the final form of this PR. Except maybe adding @throws StreamsException to the allLocalStorePartitionLags javadocs, but not sure that warrants an entire PR

Ack. In 2.5 we use the AdminClient directly in allLocalStorePartitionLags and don't apply a timeout on get(). -- Might still be worth to do quick PR to update the JavaDocs :)

Fair enough: https://github.com/apache/kafka/pull/8772


RN v0.14.0-rc running on device, “it looks like the implementation of DEV has changed”

I'm trying to load bundle from iphone disk with v0.14.0-rc.
I uncommented this line in AppDelegate.m:
jsCodeLocation = [[NSBundle mainBundle] URLForResource:@"main" withExtension:@"jsbundle"];
then I created the bundle with this command:
react-native bundle --entry-file index.ios.js --platform ios --bundle-output main.jsbundle --verbose
Finally, after installing the app on the iphone, it starts and then hang on white screen, and here is the error screen from xcode:

Thank you for your help :)

Look at #3782

Closing this since it's tracked in #3782 and will be fixed in 0.14 final (coming in a few days).


[ACS] The create command doesn't support auto-completion for its orchestrator-type argument

The help text for the az acs create command doesn't mention what the allowable values are for the --orchestrator-type argument  (other than mentioning that dcos is the default value), and that argument also doesn't seem to support auto-completion. It would be great if it was clearer that Swarm, DC/OS and Kubernetes were supported, and if so, the exact values that the command expected in order to use them.

@brendanburns to comment

@lostintangent thanks for the feature request, definitely worth doing.

@mayurid I can't seem to assign this to myself, can someone else do it?

@brendandburns : done and labelled it correctly too.

@brendandburns , make sure the swagger (and python sdk) exposes the new enum member, move all acs commands/params from vm to acs module like we planned, then the auto-completion will work automatically

Cool, swagger update is merged, in waiting on the pypi update will get to
it early next week.
Thanks
Brendan
On Fri, Nov 11, 2016, 10:32 PM Yugang Wang<EMAIL_ADDRESS>wrote:

@brendandburns https://github.com/brendandburns , make sure the swagger
(and python sdk) exposes the new enum member, move all acs commands/params
from vm to acs module like we planned, then the auto-completion will work
automatically
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Azure/azure-cli/issues/1304#issuecomment-260105324,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABOYXkP44_9qk6cl98ZAj_g2EB4SxEDcks5q9V2TgaJpZM4KwS2H
.

fixed through #1326

@brendandburns Thanks for the really fast turnaround!


React step 4

Closes #
{{short description}}
Changelog
New

{{new thing}}

Changed

{{change thing}}

Removed

{{removed thing}}

Absolutely vexed trying to find the root cause of this minor discrepancy in styling:

After hours of comparing my code with the tutorial and with other forks, I cannot find it.
I think some files changed between step 3 and step 4.  It got much worse going from step 4 to step 5.  Some files like .env simply disappeared at the start of step 5.

Correct - since you didn't start with a checked out branch for this step, you have mishmashed step 3 and step 4 files.
To solve:

Close this PR.
git checkout master
git branch -D react-step-4
git fetch upstream
git checkout -b react-step-4 upstream/react-step-4

Then follow the the rest of the instructons in step 4.

Closing PR to restart step 4


mmseqs alignall not behaving as expected?

Hi,
I am interested in computing all pairwise sequence identities in a given cluster. I believe mmseqs alignall accomplishes this task, but I am getting unexpected results.
Using the "examples" folder that was downloaded during the mmseqs installation process, I created the following clusters:
mmseqs createdb examples/QUERY.fasta queryDB
mmseqs cluster queryDB queryDB_clu tmp --min-seq-id 0.3 -c 0.8
mmseqs createtsv queryDB queryDB  queryDB_clu queryDB_clu.tsv
I then use mmseqs alignall to compute all pairwise sequence identities in each cluster:
mmseqs alignall queryDB queryDB_clu queryDB_alnall
mmseqs createtsv queryDB queryDB queryDB_alnall queryDB_alnall.tsv
OFS="\t" awk 'FNR==NR{a[$1]=$2;next}{$3=a[$3]; print}' queryDB.lookup queryDB_alnall.tsv > queryDB_alnall_newid.tsv
Unfortunately, the pairwise sequence identities that get calculated seem incorrect. For instance, when you compare the sequence P40148 against itself, I get a sequence identity less than 100% (i.e., 93.1%). Below is an example line from the output file queryDB_alnall_newid.tsv:
P40148 P40148 P40148 192 0.931 2.952E-59 -1 96 97 -1 96 97
Alternatively, I tried specifying alignment mode of 3 (because I think that's also the default of mmseqs cluster):
mmseqs alignall queryDB queryDB_clu queryDB_alnall --alignment-mode 3
Then I strangely get 0% sequence identity for all pairwise comparisons. E.g,:
P40148 P40148 P40148 192 0.000 2.952E-59 0 96 97 0 96 97
I am a non-scientist learning how to use mmseqs, so I may be missing something obvious. I would be very grateful for your help!

You need to pass either -a (compute backtraces) or --alignment-mode 3 (compute full alignment). This is a long standing trap that we weren't able to solve well yet.
See https://github.com/soedinglab/MMseqs2/wiki#how-does-mmseqs2-compute-the-sequence-identity

Thank you so much for your help!
I have tried adding --alignment-mode 3, but I am still strangely getting 0% sequence identity for all pairwise comparisons. Please find below the exact steps I'm following:
Using the "examples" folder that was downloaded during the mmseqs installation process:
mmseqs createdb examples/QUERY.fasta queryDB
mmseqs cluster queryDB queryDB_clu tmp --min-seq-id 0.3 -c 0.8 --alignment-mode 3

mmseqs alignall queryDB queryDB_clu queryDB_alnall --alignment-mode 3
mmseqs createtsv queryDB queryDB queryDB_alnall queryDB_alnall.tsv
OFS="\t" awk 'FNR==NR{a[$1]=$2;next}{$3=a[$3]; print}' queryDB.lookup queryDB_alnall.tsv > queryDB_alnall_newid.tsv

Then in the output file queryDB_alnall_newid.tsv, I observe the following. I copied below the first few lines of the file. I believe v5 is sequence identity and everything is zero in that column:

v1
v2
v3
v4
v5
v6
v7
v8
v9
v10
v11
v12

A7TBS3
A7TBS3
A7TBS3
127
0
1.39E-37
0
56
57
0
56
57

A0A0Q2X9M7
A0A0Q2X9M7
A0A0Q2X9M7
901
0
0
0
469
470
0
469
470

E1WZ27
E1WZ27
E1WZ27
1191
0
0
0
584
585
0
584
585

A0A0A6L293
A0A0A6L293
A0A0A6L293
3034
0
0
0
1489
1490
0
1489
1490

A0A0G2K4F4
A0A0G2K4F4
A0A0G2K4F4
342
0
0
0
167
168
0
167
168

A0A0B0N589
A0A0B0N589
A0A0B0N589
632
0
0
0
315
316
0
315
316

B2S328
B2S328
B2S328
538
0
0
0
272
273
0
272
273

E3LCT8
E3LCT8
E3LCT8
841
0
0
0
408
409
0
408
409

M4E0S7
M4E0S7
M4E0S7
432
0
0
0
213
214
0
213
214

H2W634
H2W634
H2W634
681
0
0
0
335
336
0
335
336

A0A0D2KX42
A0A0D2KX42
A0A0D2KX42
673
0
0
0
340
341
0
340
341

A5F385
A5F385
A5F385
500
0
0
0
252
253
0
252
253

P84927
P84927
P84927
63
0
5.54E-16
0
30
31
0
30
31

A0A091DCV0
A0A091DCV0
A0A091DCV0
233
0
0
0
114
115
0
114
115

C7P6Z6
C7P6Z6
C7P6Z6
1082
0
0
0
520
521
0
520
521

B0BCY9
B0BCY9
B0BCY9
1979
0
0
0
956
957
0
956
957

A0A146V1G4
A0A146V1G4
A0A146V1G4
769
0
0
0
409
410
0
409
410

A0A0C3SGE9
A0A0C3SGE9
A0A0C3SGE9
1246
0
0
0
630
631
0
630
631

U5G6K1
U5G6K1
U5G6K1
1083
0
0
0
530
531
0
530
531

A0A017PM95
A0A017PM95
A0A017PM95
487
0
0
0
243
244
0
243
244

A0A017PM95
A0A017PM95
A0A0W1BG93
164
0
0
5
239
244
9
240
249

A0A017PM95
A0A0W1BG93
A0A017PM95
164
0
0
9
240
249
5
239
244

A0A017PM95
A0A0W1BG93
A0A0W1BG93
499
0
0
0
248
249
0
248
249

I get sensible values for v5 and v6 when I execute the same steps you are running. Which MMseqs2 version or commit are you using?

Thank you for your quick reply. My apologies, I'm not sure how to check the version? I believe I had compiled from source under Linux back in February 25, 2021. Do you recommend that I delete my current installation and re-install everything? (Or is there a way to easily update mmseq2? I can't seem to find such information in the user guide.)
One thing that makes me hesitant about deleting everything is that I had downloaded Swiss-Prot using mmseqs databases back in February 2021 and I'm not sure if I can download the same dataset again since Swiss-Prot has been updated since then. Thanks again!

running mmseqs version should return either a version number or a git commit. I tried running your commands with the latest release (13.45111) and with the latest git commit and both seem to work.

Thank you! Hmm, so I get this message:
MMseqs2 Version: GITDIR-NOTFOUND
I looked through the other Q&A, it looks like this might have been because I downloaded a ZIP folder? (I unfortunately don't recall because I had downloaded mmseq 9 months ago.)

Compiling from zip results in this GITDIR-NOTFOUND.
I would recommend to download MMseqs2 again (the latest release from conda or from github releases or compile from source) and try again.

I will try that over the next few days and keep you posted. Thanks so much again for all of your quick replies -- much appreciated.

Hi Milot -- thank you so much for your help a few weeks ago. I just wanted to let you know that re-downloading MMSeqs2 helped fix this issue. Thanks again for your quick and detailed replies!


tls-server-name should be "kubernetes"

Proposed change
In "Tutorial: Expose a local Kubernetes API Server", there is a section shows us how to modify kubeconfig file with the new endpoint. The current value (k3s.example.com) of tls-server-name is misleading. It should be kubernetes or one of the kubectl cluster-info suggests:  kind-control-plane, kubernetes, kubernetes.default, kubernetes.default.svc, kubernetes.default.svc.cluster.local, localhost.
Context
I created a cluster using KinD on my Ubuntu machine. Following the tutorial, everything goes fine. However, if the value of tls-server-name is configured to the subdomain of the public VM's IP, kubectl will be unable to connect to the private k8s cluster due to certificate issue.
Your Environment

inlets-pro version: 0.9.3-rc1

Are you using inlets? Yes

Operating System and version (e.g. Linux, Windows, MacOS): Ubuntu 18.04

Thank you. Could you send a quick PR for this?

Fixed.


Cient connection establishment is not cancellable

Expected behavior
When connecting a client, it should be possible to cancel the connection establishment while it is in progress (if the underlying tansport supports it).
Actual behavior
The promise returned by Bootstrap#connect is marked as uncancellable by all transport implementations so invoking cancel() on it has no effect. This is so despite all¹ transport implementations having a listener on the promise which handles cancellation (see e.g. here for the NIO transport).
Marking connect promises as uncancellable was introduced by this patch. Oddly, it claims to "Make most outbound operations cancellable" but then goes on to say "Call setUncancellable() before performing an outbound operation" which appears to do the exact opposite. I already brought this up with @trustin on Discord and he figured he did this because it's not possible to interrupt a connect() syscall once invoked. However, this is not the case for all non-blocking socket types for which this can be achieved by simply calling close() on them (which is what the listeners mentioned would above do). I have in fact tested this in practice with both the NIO and epoll transports and it works just fine. I suspect that the patch was a bit too sweeping in disabling cancellation globally like that. Also note the existence of io.netty.testsuite.transport.socket.SocketConnectionAttemptTest#testConnectCancellation which indicates that cancellation of connection attempts is/was intended to be supported.
1: Except for the deprecated OIO transport which doesn't support it due to its blocking nature.
Steps to reproduce
See below
Minimal yet complete reproducer code (or URL to code)
My (very simple) patch for enabling cancellation of connection establishment for the NIO and epoll transports is available here - it can probably also be extended to kqueue but I have no system to test this on right now. I ensured that it works as expected in two ways:

I modified io.netty.testsuite.transport.socket.SocketConnectionAttemptTest#testConnectCancellation to error out in the else branch (i.e. the one that says // Cancellation not supported by the transport). The test then failed without the patch and passed with the patch.
I made a connection attempt to a TCP server which never calls accept().  Via ss I observed the connection attempt being stuck in SYN-SENT state. I then invoked cancel() on the promise returned by Bootstrap#connect. Without the patch, the connection would stick around until the connect timeout (default 30 seconds) expired. With the patch, the connection immediately disappeard.

Netty version
4.1.107
JVM version (e.g. java -version)
19.0.2
OS version (e.g. uname -a)
Linux 6.7.2 (NixOS)

FWIW, I would be happy to see this changed in Netty 4 still. Context: https://github.com/clj-commons/aleph/pull/714
Also, it's worth noting that the current behavior somewhat masks a resource leak in the sense that canelling is ineffective and connections will stick around until the connect timeout expires. An application which runs on the default of 30s (which I assume is quite likely to be the case) and establishes and quickly cancels a lot of connections in a short time will unknowingly tie up a lot of file descriptors this way.

@DerGuteMoritz thanks for reporting... This should be fixed by https://github.com/netty/netty/pull/13849

@normanmaurer Great, thanks a lot for the quick response and glad you agree to include it in the next release! :pray:


Error property athome null

Keep getting error property athome null
Any help as to what this is

The exact error is
Error occurred by getting user
TypeError cannot read property ‘athome’ of null

Im currently working on v5. Should be fixed now

Hey, i opened a new issue with instructions to test the beta version!
https://github.com/SeydX/homebridge-tado-platform/issues/16

should be fixed in v5


Preparation for Core release 1.32.0

API change check
API changes are not detected in this pull request.


crypto/cipher: Examples use an ASCII key

The examples in the crypto/cipher package use ASCII for the encryption key, which limits the range of keys and makes them easier to crack. We should hash a password and use the hash as a key, or just use 16/32 bytes of random data.
(via @FiloSottile who suggested this to me last night).

The frequent exhortations to add authentication in these examples are also making me nervous. Authenticating + attaching to the message isn't trivial. Wondering if we should steer people to other primitives instead.


use larger width/height defaults for non-interactive consoles

When a user is redirecting output to a file, or perhaps running a job non-interactively (say, via cron), arguably, it is surprising when output (such as a rich.table) is truncated to a default size of (80, 25). This change preserves that default Console.size() -> width=80, height=25, but uses larger defaults when Console.is_interactive == False.
Type of changes

[ ] Bug fix
[ ] New feature
[ ] Documentation / docstrings
[ ] Tests
[x] Other

Checklist

[x] I've run the latest black with default args on new code.
[ ] I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
[x] I've added tests for new code.
[x] I accept that @willmcgugan may be pedantic in the code review.

Description
I am not at all sure that I'm solving the right problem here or going about it correctly, but I prefer describing what I'm trying to accomplish, via code (pull request) rather than simply descriptively, via a project issue. The basic idea here is that I personally find the following result surprising:
given the following table test (basically pulled directly from the example in the documentation, here: https://rich.readthedocs.io/en/stable/tables.html#tables):
#!/usr/bin/env python
from rich.console import Console
from rich.table import Table

table = Table(title="Star Wars Movies")

table.add_column("Released", justify="right", style="cyan", no_wrap=True)
table.add_column("Title", style="magenta")
table.add_column("Box Office", justify="right", style="green")

table.add_row("Dec 20, 2019", "Star Wars: The Rise of Skywalker", "$952,110,690")
table.add_row("May 25, 2018", "Solo: A Star Wars Story", "$393,151,347")
table.add_row("Dec 15, 2017", "Star Wars Ep. V111: The Last Jedi", "$1,332,539,889")
table.add_row("Dec 16, 2016", "Rogue One: A Star Wars Story", "$1,332,439,889")
table.add_row("May 22, 2026", "As Yet Unnamed Amazing Next Star Wars Movie With A Really Long Name", "$999,999,999,999")

console = Console()
console.print(table)

I like that it truncates the table to my terminal width/height when running it interactively (say, ./print_rich_table.py), but would prefer that it doesn't truncate and/or wrap when non-interactive, for instance, when redirecting to a file (./print_rich_table.py > star_wars.txt), or perhaps running a job via cron or in some other way where the expectation is for the handling of display constraints to be handled by some later user who may not be the original script runner.
I've tested this change by shrinking/growing my terminal and making sure the old behavior is preserved, as well as by updating tests/test_console.py with some new cases.
I'm happy to try to take another approach here if this doesn't make sense. Also, of course, I picked the new defaults out of thin air (just trying to come up with some reasonably large numbers to prove out the strategy).

This is a bad idea. You wouldn't want a rule or table to be 32767 characters long.

Ah, I fully concede that this might be bad to handle it this way, but let me try to explain my actual end goal here a little bit more clearly, in case there's another way towards what I'm trying to achieve...
In our environment, we use rich.table (which we love!) in various ways. It is incredibly helpful for rich to fit output tables to the width of a terminal when a user is doing ad-hoc interactive work. However, we have many use cases where the output of a job that has rich.table components, goes to a text file for later analysis/viewing. We typically don't want any sort of truncation of that data in those scenarios. That is, the user would happily resize their terminal and/or scroll horizontally to look through the table. They generally want to see all of the field values, without any truncation. Consider especially the use case where we can't predict in advance what the min/max column/width boundaries are expected to be, and perhaps even won't be looking at the output until weeks after it was saved. Of course, one could argue that rich.table isn't the right tool for this job, and outputting as some sort of standard table instead (such as csv), but there are many use cases where it's just simpler or more user friendly for there to be a expectation of visual consistency (ie, the user looks through these sorts of tables all the time, and is more efficiently able to glance at it when it has the same look and feel as what they see interactively).
My assertion is really that I don't believe that console dimensions or a default (such as 80x25) should be used at all in those cases (ie, my large number was "pseudo-infinite" for practical purposes).
We can certainly work around this, but it is relatively burdensome to the developer to manage this on a per-script basis, rather than it being a core behavior of rich, or at least modifiable at a high level (perhaps a flag that gets passed to the rich.console.Console() constructor, optionally, that maybe defaults to the current behavior, and changes the behavior if specified).

You can set the env var COLUMNS to your desired width when running non-interactively. Would that work for you?

Yeah, we have various viable workarounds such as setting $COLUMNS, or modifying the behaviour of individual scripts (typically by branching on console.is_interactive, setting console.width and console.height to something different - very similar to the strategy demonstrated in the PR). This proposal was really about suggesting that it might make sense to be a core feature of rich (in some way), but I was fully anticipating the possibility that it wouldn't be desirable. That's fine with me! It's food for thought more than anything, and in case other users encounter similar pain points surrounding this topic, they can use it as reference.

Guess I happened into this the day after it has been closed.  My use case is exactly as @ttarabula described: rendering tables to stdout in a non-interactive manner (a Chronical scheduler job.)  The renderer is defaulting to 80x25, badly truncating the table values and width.
What I would hope for: that rich.table have some means of being configured in a "do not truncate values" and "do not limit table width" mode, yet still narrow the columns to the widest values.


Prefer @query over ref()

Lit team prefers suggesting @query over ref. Though this example is simple enough to just do this.renderRoot.querySelector('input')!.focus() as well, the title for this section is "dom-ref"

Thanks for your contribution !
You put some TypeScript syntax in a JS file. That's why the CI failed

@e111077 please remove HTMLInputElement

didn't realize it was a JS file!


[FR] - Provide governance query for stake distribution

Internal/External
Internal   if an IOHK staff member.
Area
Token Locking    Related to Token Locking (Allegra).
Native Tokens    Related to Native Tokens (Mary).
Plutus    Related to Plutus Scripts (Alonzo).
Other   Any other topic (Delegation, Ranking, ...).
Describe the feature you'd like
This is provided by consensus in: Added GetSPOStakeDistr query. We need to provide a governance query to get this data for end users using CLI.
Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Additional context / screenshots
Add any other context or screenshots about the feature request here.

Still more PRs to merge


Resource commands menu goes off the bottom of the screen

Originally reported in https://github.com/dotnet/aspire/pull/2572#pullrequestreview-1915908165:

I filed https://github.com/microsoft/fluentui-blazor/issues/1640 to try to get VerticalThreshold added as a parameter to FluentMenu. If/when that happens, we can set it and this issue should be fixed.

Note to self that the fix needed should be in 4.5 which we updated to, so this can move forward.


CPU and NET won't reset if used > max after 3 days

I have to wait until the cpu price go down and make a transaction to reset the cpu & net used after 3 days.
This should be changed:

Auto reset after 3 days
or
Transactions should be process and the cpu & net will reset after 3 days even if used > max

    "cpu_limit": {
        "used": 15514,
        "available": 0,
        "max": 14276
    }

CPU used is calculated using an exponential moving average with a window size of 24 hours (in milliseconds) divided by the block interval, which is 500 ms.  There is no process in the system that recalculates the usage on some scheduled interval.  Instead it is calculated the next time you submit a transaction.  Try submitting a new transaction and see what the values are.

The usage values will be refreshed if you push a transaction on that account. It will not be automatically recalculated.

@taokayan, how are we supposed to "push a transaction" if the transactions fail due to "Insufficient CPU"?


"Cannot resolve..." with node-ninja, node-gyp and mocha

Hi,
I am trying to compile an application that uses couchbase, and other modules, which in turn use node-ninja, node-gyp and mocha.  I get warnings from all three of these similar to the following:
Warning Cannot resolve ''./' + c'
  node_modules/node-gyp/lib/node-ninja.js
  Use a string literal as argument for 'require', or leave it as is and specify the resolved file name in 'scripts' option.

Looking at the node-ninja code, this comes from the following segment:
 commands.map(function (c) {
    return '    - ' + c + ' - ' + require('./' + c).usage
 })

I've read the "Assets" and "Scripts" sections of the pkg Readme but I'm still confused about how to resolve this.  Do I need to find out every script that may possibly be in the "commands" array (used in the segment about) and specify each of these in "assets"?  Has anyone else tried compiling an app that uses one of these libraries?
Thanks,
Giles

Ok. Aside that warnings does the compiled application work well? Or has any run time issues?

Thanks for the reply.  At the moment, I haven't reached the point where I have a complete executable so I can't answer this yet.  My node server does run but I'm not yet including my website files as assets so get an ENOENT error when navigating.
I just wanted to check whether there was a standard solution to the node-ninja issue whilst I was trying to get the rest of the app working.  I'll update when I have fixed the website assets and updated all paths, then I'll know more.

Hi @igorklopov,
I have now packaged all relevant files and everything seems to work as expected!  My compiled output consists of only four files - the executable and three .node files (which, as I understand it, can't be enclosed in the package at the moment).  The original server directory contains several hundred files so this really is awesome!
I still get the warnings from node-ninja, node_gyp and mocha, but so far it appears I can ignore them as I haven't seen any runtime issues.
Thanks for the good work on pkg.
Giles

@gillez Hi! I am very glad! I constantly make pkg suppress warnings from packages that are known to work well (along with compatibility tests https://github.com/zeit/pkg/tree/master/test/test-79-npm). So, probably you will not see those warnings in future releases.

Thanks for your help Igor.
I've got a separate issue now related to cross-platform building but I will create a separate ticket for that.

Closing this issue as I am happy with Igor's answer.


compilation error due to -Werror

I am on Archlinux using GCC 10.2 and get the following error while building ROS:
--- stderr: mimick_vendor
In file included from /usr/include/errno.h:25,
                 from /opt/ros/build/mimick_vendor/mimick-ext-prefix/src/mimick-ext/include/mimick/mock.h:27,
                 from /opt/ros/build/mimick_vendor/mimick-ext-prefix/src/mimick-ext/include/mimick/mimick.h:401,
                 from /opt/ros/build/mimick_vendor/mimick-ext-prefix/src/mimick-ext/sample/strdup/test.c:1:
/usr/include/features.h:397:4: error: #warning _FORTIFY_SOURCE requires compiling with optimization (-O) [-Werror=cpp]
  397 | #  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
      |    ^~~~~~~
In file included from /usr/include/errno.h:25,
                 from /opt/ros/build/mimick_vendor/mimick-ext-prefix/src/mimick-ext/include/mimick/mock.h:27,
                 from /opt/ros/build/mimick_vendor/mimick-ext-prefix/src/mimick-ext/include/mimick/mimick.h:401,
                 from /opt/ros/build/mimick_vendor/mimick-ext-prefix/src/mimick-ext/test/test.c:1:
/usr/include/features.h:397:4: error: #warning _FORTIFY_SOURCE requires compiling with optimization (-O) [-Werror=cpp]
  397 | #  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
      |    ^~~~~~~
cc1: all warnings being treated as errors

I noticed that this error does not happen when Mimick is built standalone. Closing.


Bigquery: Expose CMEK functionality for ML models.

Fixes #6300

Thanks! Please add some minimal testing e.g. ModelInfoTest to ensure we propagate through builders, etc.

Done, PTAL.


Unable to build from source (iptables issue)

As of go1.17 the go get command mentioned doesn't work and has been deprecated, instead go suggests to use go install, which fails.
When I'm trying to build from source I encounter this issue:
$ make all source-ip=256.256.256.256/32
sudo iptables -A OUTPUT -p tcp --tcp-flags RST RST -s 256.256.256.256/32 -j DROP
iptables v1.8.7 (legacy): host/network `256.256.256.256' not found
Try `iptables -h' or 'iptables --help' for more information.
make: *** [Makefile:10: all] Error 2

OS: fedora 36

The error is coming from iptables. It is complaining that you are inputting an invalid ip address to scan from. The program expects you to provide a valid source ip address that the scanner will scan from.


chore(#293): move Druxt Views nuxt module files

Types of changes

[ ] Bug fix (a non-breaking change which fixes an issue)
[ ] New feature (a non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[x] Other

Description

Moved the Nuxt module files into a nuxt directory.

Checklist:

[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly. (PR: #)
[ ] I have added tests to cover my changes (if not applicable, please state why)
[ ] All new and existing tests are passing.

Screenshots/Media:

Code coverage has gone down as there are less lines due to this commit, so nothing is less covered.


Refactor JavaUtil and OaspUtil in templates

The util classes already growed too much, most probably as of their very generic name.
We should refactor the classes to make them easy to be discoverable, readable and less complex in case of method length. For example take getConnectors methods. They can be easily refactored to multiple methods.

Finished as of #712


BREAD translation

Version information

Laravel: v5.8
Voyager: latest
PHP: 7.2
Database: [type] [version] (e.g. MySQL 8.0)

Description:
A clear and concise description of what the problem is.  Please also explain why this doesn't fit into the defined issue templates
Additional context
Add any other context or screenshots about the feature request here.

how to translate these column names?
thanks

As per the issue template, questions should be asked in our Slack group.


"Getting Acquainted" quest fails after a raid

SPT Version
3.10
Project Type
No response
Expected Result
After accepting the quest, it shouldn't fail unless you kill Zryachiy.
Actual Result
After accepting the quest and running the raid, the quest fails.
Steps To Reproduce

Accept "Getting Acquainted"
Enter any raid
Extract from the raid

Server Log
server-2024-12-28.log
BepinEx Log
LogOutput.log
Client Log
2024.12.28_16-18-49_<IP_ADDRESS>.33420 traces.log
Player Profile
677024ce0004932a0cc764a2.json
Screenshots
No response

According to the user in #community-support who originally reported it, this was a common issue in Live PVE in the previous wipe.

BSG refactored quest handling in 0.15.5, and added a call that triggered a condition check before the condition handlers were setup. They've resolved this in 0.16 with a simple removal of a call to a function.
0.15.5:

0.16:

I might dig into this to see if we can remove that call without breaking everything


Running GoAccess from CLI displays command options

Hello,
when I launch the goaccess command like "goaccess /var/log/apache2/access.log -a -o report.html" on OSX 10.12,  only list of commands appears.
any ideas ?
regards,
Denis

Are you on the latest version? goaccess -V

version is 0.9.6 !

Please upgrade to the latest version OR you can run it as:
goaccess -f /var/log/apache2/access.log -a > report.html

It works with the command -f , thanks a lot :)


Sometimes strange visual effect with source dates enabled

Sometimes the editor windows acts strange (with source dates enabled).
Scrolling out and in again to this line shows it normal again.

QCCSID: 1141
Features:

git: false
grep: false
tn5250: false
setccsid: true
GENCMDXML.PGM: false
QZDFMDB2.PGM: true
iconv: true
attr: true

SQL enabled: false
Source dates enabled: true

Variants
{
  "american": "%23@$",
  "local": "%23§$"
}

Errors:
[
  {
    "command": "/QOpenSys/usr/bin/qsh",
    "code": 1,
    "stderr": "CPC2206:  Eigentumsrecht für Objekt QRNFER in QTEMP Art *USRSPC geändert.\nCPC2206:  Eigentumsrecht für Objekt QRNFEF in QTEMP Art *USRSPC geändert.\nRNS9308:  Umwandlung gestoppt. Fehler mit Bewertungsstufe 30 im Programm gefunden.\nCPC2206:  Eigentumsrecht für Objekt RETURNCODE in QTEMP Art *DTAARA geändert.\nCPC0904:  Datenbereich RETURNCODE in Bibliothek QTEMP erstellt.\nRNS9310:  Umwandlung fehlgeschlagen. Programm AC17131 in Bibliothek EM01SRC nicht erstellt.\nCPC2206:  Eigentumsrecht für Objekt QZSHSYSTEM in QTEMP Art *USRSPC geändert.",
    "cwd": "/home/HEL"
  },
  {
    "command": "/QOpenSys/usr/bin/qsh",
    "code": 1,
    "stderr": "CPC2206:  Eigentumsrecht für Objekt QRNFER in QTEMP Art *USRSPC geändert.\nCPC2206:  Eigentumsrecht für Objekt QRNFEF in QTEMP Art *USRSPC geändert.\nRNS9308:  Umwandlung gestoppt. Fehler mit Bewertungsstufe 30 im Programm gefunden.\nCPC2206:  Eigentumsrecht für Objekt RETURNCODE in QTEMP Art *DTAARA geändert.\nCPC0904:  Datenbereich RETURNCODE in Bibliothek QTEMP erstellt.\nRNS9310:  Umwandlung fehlgeschlagen. Programm AC17131 in Bibliothek EM01SRC nicht erstellt.\nCPC2206:  Eigentumsrecht für Objekt QZSHSYSTEM in QTEMP Art *USRSPC geändert.",
    "cwd": "/home/HEL"
  },
]

Weird!
Any notes on how I can recreate this? We have a PR in the works which make fix this also.

This happens with me as well.  It happens especially when deleting lines of code with the delete key.  I normally will delete lines of code with the backspace.  It seems to be work better.  But occasionally it will happen just paging down through the code.   When it gets bad, I normally restart VSCode and it will reset

@Hadkins2 I will look into source dates being affected with the source dates. Thanks!

This also happens.

@fheldt @Hadkins2 Is this still an issue?

@fheldt @Hadkins2 Is this still an issue?

I haven't seen the source date becoming 2 digits issue lately, but the source date disappearing and the code line shifted left happened to me today.  It was the last line of source in the member.
Howard

@Hadkins2 may I recommended changing edit mode to diff mode in the connection settings?

The issue described by @Hadkins2 just happened to me today, with Code for IBM i 1.7.11. It's not that the gutter date becomes 2 digits. It's that the gutter somehow effectively moves over to the left by some amount of pixels (or other arbitrary graphical measure) and there isn't a way to bring it all the way back into the displayable area:

I think this is related to horizontal scrolling somehow. Normally, when you have super-long lines (in other files, not handled by Code for IBM i) and you cursor your way far to the right, the "viewport" moves to the right automatically, to keep the cursor in view. If you get to the end of such a line and then right-arrow one more time to go to the beginning of the next line, the viewport resets so that the leftmost part of your file is visible.
But when the source date gutter is involved, sometimes this doesn't quite work.
Incidentally, I feel like horizontal scrolling in general (at least with source date gutter enabled, which is the only way I ever use it) is a bit wonky with Code for IBM i. I am always surprised whenever the viewport jumps to the right for any reason, because the lines of code are never anywhere near the right edge of my screen. There aren't any source files defined with wide enough records to ever trouble the horizontal capacity of my editing window. Yet there is this unsettling jumping.
I can only imagine it has something to do with the editing process. In the course of deleting, say, you could wind up joining two lines. Of course it's only a temporary situation, but for that short time where you have multiple lines' worth of content on one line, it may indeed be too long to fit on the screen, and then the chances of odd things happening goes up dramatically. I suspect this is the case even when the cursor isn't anywhere near the right edge of the screen, because, as I said, I am always surprised when it happens. Even for those brief moments when I may have too much content on one line, I don't think I ever hit the End button or do anything else that should necessitate the viewport having to move horizontally at all. With the cursor still well within the range of columns that would fit in the normal viewing area, I press Enter to restore the situation to all lines being a normal length.
Oh, and now in the course of writing this comment, I switched back to VS Code and the gutter is fully visible again. I didn't do anything in VS Code, just switched back to it after a time away from it. It is a graphical rendering thing. For me, this underscores just how brittle and finicky (and yet kind of awesome) the whole gutter concept is (in VS Code intrinsically, not necessarily anything Liam has control over). I wonder if this is the kind of thing that Theia, with its theoretically deeper UI customizability, would allow to be implemented in a more straightforward and reliable way.
Sorry for my extended ramblings. If you've made it to the end, thanks for reading!

Yes, I'm seeing it in Windows 11. It might be hard to reproduce. I can cause the weirdness at will right now in one member that I have open (has been open for a while, with lots of unsaved edits) but not in another (that I only just opened).
In the one that's having the issue, I can cause the weirdness as follows:

Put the cursor on column 1.
Select a bunch of lines by holding down Shift and pressing the down arrow a few times, so the cursor is still on column 1.
Press Delete.

Expected behavior: The selected lines are deleted, the cursor stays on column 1, the viewport stays where it is, and the source dates stay where they are.
Actual behavior (in one member): The selected lines are deleted, the cursor stays on column 1, the viewport jumps far to the right - which means I can't even see the cursor anymore! - and when I move the cursor just to get the viewport to come back to the left, the source dates are obscured except for a little bit of the rightmost digit(s).
Actual behavior (in a different member): Same as expected behavior.


FlameGraphs with stacks higher than 2000 frames are not rendered correctly

The canvas with the height > 32767px does not work in Chrome (and probably oher browsers).

I noticed that the tops of my graphs are often seemingly empty. Zooming in I would find a single stack that's super high, but only a single pixel wide. Maybe having a higher minimum width would also help with the height of the canvas?

Yes, that's what --minwidth argument does. If you specify --minwidth 0.1, frames with less than 0.1% samples will be omitted, and this will often decrease the graph height.
Since the introduction of HTML Flame Graphs, the default minwidth was dropped to zero, because small frames no longer affect the rendering performance as much as with SVG. Even when barely visible, small frames are useful for understanding the entire profile, in particular, when searching for a specific method to find out its cumulative impact.

Makes sense, I'll just change my minwidth setting then.


Issue with imagePullSecrets after upgrading to version 3.X

First of all big thanks with the common chart!
Details
I am using the app-template chart for one of my private projects. The image for this is in a private docker hub project so need to use the imagePullSecrets property in the container config.
The imagePullSecrets property works in the 2.x template versions but upgrading from 2.6.0 to 3.0.4 gives an error.
Helm values:
controllers:
main:
containers:
main:
env:
TZ: Europe/Stockholm
image:
imagePullSecrets:
- name: dockerhub-image-pull-secret
repository: {image}//removed
tag: v1.0.3
command:
helm upgrade --history-max=5 --install=true --namespace=prod --timeout=10m0s --values=/home/shell/helm/values-app-template-3.0.4.yaml --version=3.0.4 --wait=true home-proxy /home/shell/helm/app-template-3.0.4.tgz
Error:
2024-03-16T19:24:53.102001161+01:00 Error: UPGRADE FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
2024-03-16T19:24:53.102019886+01:00 app-template:
2024-03-16T19:24:53.102024624+01:00 - controllers.main.containers.main.image: Additional property imagePullSecrets is not allowed
Is imagePullSecrets still supported? Should the the imagePullSecrets be moved as part of the changes in to version 3.X?
Best Regards, Marky

Hi, thanks for raising the issue! In app-template / my common library imagePullSecrets are (and have been since V2 iirc) specified on the pod level, they should be set by defaultOptions.imagePullSecrets or something like the following if you don't want to "globally" set them:
controllers:
  main:
    pod:
      imagePullSecrets: []

Hi
Thanks both of the options works for my use case so I could update today.


[BEAM-7060] Prefer inspect module over funcsigs

cherrypick of https://github.com/apache/beam/pull/9562

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

[ ] Choose reviewer(s) and mention them in a comment (R: @username).
[ ] Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
[ ] If this contribution is large, please file an Apache Individual Contributor License Agreement.

R: @robertwb

CC: @markflyhigh

closing - dupe of #9591


[Missing SNMP Polling Profile]

Device Vendor: Netscout
Device Model: Netscout Arbor Edge Defense 2600
System Object Identifier (SysOID):
List of critical metrics that you expect in the profile (does not need to be all-inclusive, just a general idea of your expectations):

If there is a New Relic technical support case related to this request, please provide the case number:
Please provide a public gist with a sanitized (sensitive information removed) SNMP walk of the device. Providing MIB files is not a substitute for an SNMP walk and failure to provide one here will result in delays for closing this issue. The output of these commands shows the "true story" of what a device will respond to in SNMP and is used to ensure that our profiles are efficient and accurate for your devices.
Gist Link:

Example SNMP walk commands:
# v2c example
snmpwalk -v 2c -t 10 -On -c $COMMUNITY_STRING $IP_ADDRESS . >> snmp_walk.out

# v3 example
snmpwalk -v3 -t 10 -l authPriv -u $USERNAME -a MD5|SHA -A $AUTH_PASS -x DES|AES -X $PRIV_PASS -ObentU -Cc $IP_ADDRESS . >> snmp_walk.out

@kumar-niteesh For this one you did not attach an SNMP walk or any other identifying information to build the profile from.  I'll need you to follow the instructions from the issue template to be able to build a profile.

Device Vendor: Netscout
Device Model: Netscout Arbor Edge Defense 2600
System Object Identifier (SysOID): .<IP_ADDRESS>.4.1.18<IP_ADDRESS>.10.1
Please check the same.


'tradeOffers' event not fired reliably

Commit 2672916d7ea0a037068e6a45d1b729f96450a0ec is causing the tradeOffers event to not fire if the user received a new trade offer after handling an old one, since the amount of incoming trade offers didn't change.

Is this intended behaviour?
Should I update the _lastTradeOfferCount property manually after handling an incoming trade offer?
What was the reason behind implementing the check in the first place?

Thanks!

How frequently are you receiving a new trade offer at the same exact time as you act on one?
No, you shouldn't ever touch underscored properties.
The reason this was implemented is because sometimes Steam can continually send trade offer notification messages with the same count when nothing changed.

Here's an example scenario where this was causing issues:

Bot receives trade offer:
tradeOffers event fires with amount set to 1, _lastTradeOfferCount is now 1.
Bot acts on trade offer (accepts / declines).
Trade offer is no longer active, amount of incoming trade offers is now 0.
Bot receives another trade offer:
_lastTradeOfferCount is still at 1, thus nothing changed and event is not fired.

Hope this explains exactly what I meant.

That scenario would not cause an issue.

Bot receives trade offer

tradeOffers event fires with amount set to 1
_lastTradeOfferCount is now 1

Bot acts on trade offer
Trade offer is no longer active

tradeOffers event fires with amount set to 0 since 0 != 1
_lastTradeOfferCount is now 0

Bot receives another trade offer

tradeOffers event fires with amount set to 1 since 1 != 0
_lastTradeOfferCount is now 1

That makes sense, but the issue here is that I have never received the tradeOffers event with an amount of 0, it was only ever reporting positive values.
Quick way to reproduce this was by sending a trade offer to the bot (tradeOffers fired with 1), then cancelling the trade offer from my end (tradeOffers did not fire at all).

Yep, I see a bug. The problem is that notification messages with a count of 0 aren't being handled properly.

Alright, thanks a lot for looking into it!

This is fixed in v3.25.2.


In my-daos page, Participated DAOs always display one data

Description
In my-daos page, Participated DAOs always display one data
Steps To Reproduce

Log in...
In two different DAOs, vote
Enter my-daos page

Current Behavior

In Participated DAOs list, only display one data

Expected Behavior

Participated DAOs list should display all DAOs that user have voted or created proposals

Environment

Platform: PC
Node: v18.18.0
Browser: Chrome 126.0.6478.56

fixed confirm


Design config validation violations notification

All the config validator violations has violation type 'CONFIG_VALIDATOR_VIOLATION' but we should separate them on a more granular level, maybe per rule name instead since the FCV instance can contain multiple rules and the resulting violation file can be huge and hard to read for users.

Feedback from Forseti day customers raised this CSCC issue. We should prioritize this based on the customer feedback.


Red pjama 3b instruct

This should close https://github.com/openvinotoolkit/openvino.genai/issues/276 , I have updated the model list , and added tests taking reference from issue description at https://github.com/openvinotoolkit/openvino.genai/issues/259 , @p-wysocki @ilya-lavrenov @pavel-esir in the readme I have added Red pjama 3b separately along with that huggingface comparison has also added. please let me know if anything else can be done.

Please, resolve merge conflicts.

Please, resolve merge conflicts

@ilya-lavrenov Sorry for the delay,I have resolved the conflict. please check.

@akiseakusa could you please apply the comments?


Optimize web site for pagespeed

The site isn't optimized for web or mobile web browsers. Doing so can improve the performance and reduce the amount of data sent over the network - particularly useful for users using mobile (cell phone) networks.
The tool doesn't login to the site, however since you're using shared resources, hopefully the changes and improvements you make can pervade the entire site.
https://developers.google.com/speed/pagespeed/insights/?url=http%3A%2F%2Fallready-d.azurewebsites.net%2FVolunteers%2FDashboard

I would be glad to jump in to this project & help out with this task. Most of those issues look like they have to do with css & js not being merged, minified & cached.
It looks like there is a Gulp task that should take care of most of the merging & minification. Would it be okay to add the css & js that is being pulled from CDNs or do you prefer that to be pulled from a CDN instead?
I could add compression to the web.config file similar to HTML5 Boilerplate's suggested values https://github.com/h5bp/server-configs-iis/blob/master/dotnet 4.5/MVC5/Web.config  If this is acceptable, would you prefer these just be done in a web.release.config file?
You can also login to the site using Chrome or Firefox & run a similar Pagespeed plugin to test the pages inside the site.

ping @dpaquette for opinions on CDN vs not etc.
Thanks @mattferderer for jumping in.  I am confident that both this particular issue and the site overall is not optimized for speed and size so taking a look at that and either fixing the issues or docing the issues for you/others to address would be a great contribution


Identify duplicate emails

Due to some error (outside my rails app) I am receiving multiple emails. They end up being processed each time and I get multiple entries.
What would be a nice way to uniquely identify an email? For my purpose, the combination of the body and attachments make the email unique. Is there some other data shared by most email providers to identify emails, or, can I expect the @email.attachments to be same for the emails- then I can hash the body and the attachments together and store it.
This issue probably is more of a "question" than a "feature request", and is definitely not a "bug" :)

Well the Message-ID header should be unique for every email. You could take a look at that to determine whether you've already seen the message.

@wingrunr21 thanks! :) Just for future reference the Message-Id header can be accessed by @email.headers['Message-Id'] (and not Message-ID)