Dupe anyitems with ME Drive #2276

Closed
linkinkov opened this Issue May 11, 2016 · 72 comments

Projects

None yet
@linkinkov

Forge version: 1614
AE2 version: rv2 stable and last rv3 beta

I can post video or description of dupe, but need email or something else, for private post.

@vallard192

If you're not interesting in sharing the details here, and it's not related
to the other open dupe tickets, then it might be easiest for you to get on
IRC and share it with devs there.

On Wed, May 11, 2016 at 3:40 PM Oleg notifications@github.com wrote:

Forge version: 1614
AE2 version: rv2 stable and last rv3 beta

I can post video or description of dupe, but need email or something else,
for private post.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#2276

@linkinkov

https://www.youtube.com/watch?v=uCWCxwMPZzQ&feature=youtu.be video, please mute the sound, because they speak in russian.
To dupe need 2 players.
You can see that first player lets see terminal, second player disconect connections of cables.
Gui of the terminal dont closing for first player then second players disconect cables.
First player wait until second take to inventory all storage cells and take from terminal gui all item that he want.
Then storage cell come back into ME drive and connect cables all item in cells dont change.

Sorry for my English

@yueh
Member
yueh commented May 12, 2016

Cannot reproduce with AE2 itself.

Please try it without any bukkit fork and maybe even check for conflicts with other mods.

@linkinkov

I am testing on vanilla too, without other mods, then 1st player open the terminal and second disconect by remove the cables, gui dont close.

@mindforger
Collaborator

without other mods ... with only forge ? no cauldron or kcauldron or anything?

@linkinkov

yes, game in single with open local connections

@mindforger
Collaborator

sorry i forgot to ask: also reproduceable with rv3.beta build 6?

@linkinkov

Dont test beta build 6, but test beta build 5.

@BlayzerQ

I confirm. The player who opened the inventory, the inventory is not updated after a power failure.

@linkinkov

Also i think the main problem in this dupe - Energy Acceptor

@XFactHD
Collaborator
XFactHD commented May 14, 2016

Why should the energy acceptor be the cause?

@itskun
itskun commented May 15, 2016 edited

I confirm. 1) Working dupe, video https://youtu.be/uYg_c-7ppdM with some differencies! Energy acceptor is not main problem, because dupe works completely across all the ME network, regardless energy acceptor. It is necessary to make the interface of the ME terminal was closed, when there is no power. A terminal is not only closed but also allows to put items in a network, even when no ME drive for a long time, and allows you to store an unlimited number of items to the server is restarted. sorry for bad english).

2)working dupe https://youtu.be/M4wWRcnDj0s. Interface closes when no power, but when me drive change to cable, you will be able to enter me interface and use inventory of me drive, can put and take items!

@BlayzerQ

This work on last build.

@Innjey
Innjey commented May 15, 2016

Confirm working on rv3 beta 6. When placing any wire instead of broken ME Drive, while second is looking into terminal, it will get all information from drive and work as it. Dropped storage cell saves information too.

@yueh
Member
yueh commented May 15, 2016

As long as nobody can provide a detailed description on how to reproduce it with only AE2 and/or any conflicting mod, I consider it a bukkit issue as I am unable to reproduce it with a normal forge instance. Neither with a dedicated server nor a shared lan world.

@yueh yueh closed this May 15, 2016
@BlayzerQ

Dupe required two players. Only one - not work!

@linkinkov

@yueh why you close issue?

@bookerthegeek
Collaborator

@linkinkov He closed it because nobody has given him step by step instructions on how to do this, on a forge based server, that is not a video.

@bookerthegeek
Collaborator

Did you see the part about not a video?

@linkinkov

@bookerthegeek video have a part about

@BlayzerQ
BlayzerQ commented May 15, 2016 edited

The first player to open the terminal, and put in items
The second player turns off the power of terminal
The first player picks up items (Terminal for it continues to operate)
The second player take all cards from ME Drive
The second player connects the power of terminal
Things remained in the terminal and things are the first player in the inventory (or drop)
The second player put cards to ME Drive
Done.
http://ssmaker.ru/c473e59d.png
http://ssmaker.ru/cf563450.png

@itskun
itskun commented May 15, 2016

This dupe is not bukkit issue, it easily reproduces in single with TE, or two players in multiplayer.

@bookerthegeek
Collaborator

Thank you for the steps. I'll test tonight.

@yueh
Member
yueh commented May 15, 2016

@bookerthegeek This will not help you at all. It is the same thing I have already tested in a couple of different version and there is only a tiny bug, that the display list will not be correctly updated once an ME Drive was removed from the network. But it still needs the terminal to be connected to the network and it is impossible to dupe with it as the list for the terminals is completely different from any inventory and extracting an item will only work should the tile entity for this inventory still exist. Except should someone use pick block, but that is already duping on purpose.

To be able to extract anything from a drive, it still has to linger around as tile entity (or the cable connecting them). Therefore something must influence the normal handling of them. As they are still unable to provide any useful information like used mods, etc. Thus I simply cannot trust them it as valid report.
Like they claim that they did not use other mods, but also claim that the energy acceptor is the issue. Which requires some other mods to be present, otherwise it is non functional.
Then they do not even state which TE version they are using. As I do not know the stance of TE/cofh in regards to bukkit, I would also not be suprised should the use some patched versions of TE, cofhcore or (or any other mod). Which would not be something new for any bukkit fork.

@BlayzerQ

We dough with both modes, and without mods. Always standing alone IC2. We are on the vanilla dough, the dough on a different server cores. Tests as the energy TE, and IC. DUP worked in all cases.

@fhntv24
fhntv24 commented May 15, 2016 edited

As a proof:
==> https://www.youtube.com/watch?v=uYg_c-7ppdM&feature=youtu.be <==
That was in Curse mod pack, and it works. We was using Material Energy^Natural Capital. I also checked this in FTB Infinity evolvded, and it worked too.

Easy step by step ( IMHO ) :

  1. Build AE system, and put ME drive BETWEN (!!! IMPORTANT !!!) ME terminal & controller ( or energy acceptor ).
  2. Someone other MUST open ME Terminal
  3. Break ME drive and put cable where it was. ME terminal MUST BE OUT OF NETWORK, so it must create its own grid.
  4. ???
  5. PROFIT

This works even with creative cell, and again: me terminal must create its own grid after ME drive destruction, and AFTER that you can power it how you want, easyest way is just to connect it to same grid where it was before

@bookerthegeek bookerthegeek reopened this May 16, 2016
@bookerthegeek
Collaborator
bookerthegeek commented May 16, 2016 edited

@yueh I hate to say this But I can reproduce in vanilla with only AE2 Installed. Screenshots incoming.

Also...
@BlayzerQ Wrong, You can do this single player.
@linkinkov These are instructions. Learn please. NOT a video, but step by step guide, barney style. (luv you yueh)
@fhntv24 Good try, but needs to be done in a clean enviroment. In this case ONLY AE2. It makes it easier for the devs to reproduce and debug.

@bookerthegeek
Collaborator
bookerthegeek commented May 16, 2016 edited

This is the Yellow Network and we will be considering this the Main network. Notice the ME terminals on both ends of the network. This is important as it does not work otherwise.
http://goo.gl/QjTioH

Next I added the Portion to Remove the ME Drive and replace it with a cable. This is the Purple Network.
http://goo.gl/glyClI

The dropper on the right is to place a yellow cable down to reconnect the network.
The two panels on the left, one is a formation plane set to yellow cable, and one is an annihilation plane on a toggle bus. Redstone is so the cable is placed after the annihilation plane is no longer powered. Note block informs me when Completed

Before the Dupe
http://goo.gl/GZxb3s

After The Dupe
http://goo.gl/GRXWn1

Also, for sake of completeness....
appliedenergistics2-rv3-beta-6.jar
forge-1.7.10-10.13.4.1614-1.7.10

World Download
https://www.dropbox.com/s/q0k8ggxshuqixr9/New%20World.7z?dl=0

@yueh
Member
yueh commented May 18, 2016

Seems like your report is valid @bookerthegeek. But I am not certain that these are ultimately related.
You replace the drive directly with a cable to prevent the network from actually changing. While other one completely disconnects the terminal from the network, which requires that the actual tile entity for the drive is not destroyed or changed (to trigger a repathing). Therefore the drives may still be around somewhere, which is a completely different issue and probably cannot be resolved by us (how should we know that a tile entity should be destroyed, if it actually prevents signaling it?)

Enforcing to post a MENetworkCellArrayUpdate once a drive is removed might solve it. But then I have not found time in the last 2 months to even install eclipse + setup a dev environment for AE...
Unlikely that it will change in the near future.
And there are a bunch of conditions which needs further testing.

@BlayzerQ
BlayzerQ commented May 18, 2016 edited

We have already set for themselves, all fixed.
For the author:
NetworkInventoryHandler.class
GridStorageCache.class

@yueh
Member
yueh commented May 18, 2016

Just announcing which files you modified is not enough, you have to make the changes publicly available for any user. Otherwise it would be a license violation.

Also fixing it at this level is most likely not fixing the issue, just hiding it.
Both classes only provide a cache containing all possible inventories like cells. Therefore either it only hides them somehow and they drives still linger around actually and just wait for another way to exploit it. But if this is something you want...
Or it has a good chance of being a horrible performance issue.

@ArcasCZ
ArcasCZ commented May 18, 2016

Tested on our server and it doesn't work - when someone wrench that cable, terminal closes. Also, if you have clicked that items, it will drop on floor and when you connect cable, it will disappear from disk. So you still have same items, no duplication, no disappearing.

@cyber01
cyber01 commented May 18, 2016

Dupe works if AE uses the energy via the energy converter from other mods. all is well with AE energy.

@bookerthegeek
Collaborator

Would it be possible to just kick players from the interface when their is a physical change to the network?

@linkinkov

Its a best idea

@leagris
leagris commented May 18, 2016

What about sub betwork?
What if network A access network B though a ME terminal. It would need to be disconnected on B topology change.
But then Me toggle bus could cause a cascade of disconnect on all attached networks. Likely very laggy.

@bookerthegeek
Collaborator

Does not work across subnets.

@linkinkov

any news?

@fishka666
fishka666 commented May 27, 2016 edited

fix here - link to potentially broken stuff removed

@bookerthegeek
Collaborator

@yueh Do you mind if I make a build for this so we can test on our server?

@fishka666

on my server this is work.

@bookerthegeek
Collaborator

And that is great that it does. I would still like to test this on a larger server.

@fishka666

test it, then write about the results )

@cyber01
cyber01 commented May 27, 2016

this fix works, only there is a problem: if you add / remove any block IU network, disappear all the resources in the ME network and need to pull out and reinsert the storage cell

@bookerthegeek
Collaborator

See, that would be an issue. What we need is for on the physical network updating, to close out all open gui's. The same as if another player breaks a crafting table while your in it.

@fishka666
fishka666 commented May 28, 2016 edited

==>this fix works, only there is a problem: if you add / remove any block IU network, disappear all the resources in the ME network and need to pull out and reinsert the storage cell<==

i don't have that problem. I am using AppliedEnergistics2-rv2-stable-10, and AppliedEnergistics2-rv3-beta-6 i don't test!

@kaziu687
kaziu687 commented May 29, 2016 edited

@cyber01 the same issue present on my server (rv3-beta-6). Fixed it by adding
this.cellUpdate(null);
after
this.getItemInventory().getStorageList().resetStatus();

@yueh
Member
yueh commented May 29, 2016

Calling IItemList.resetStatus() is extremely stupid and does not solve anything. Except cause more issues as it might not repopulate the grid at all until something changes again. Which also includes any available crafting pattern.

Similarly closing the GUI is also a bad idea. The visible items are only cached and not related to any actual inventory operation. This means due to whatever reason the disks are still dangling and can be potentially access through other means. Say buses, AE2 AddOns etc. It would just be a Quick'n'Dirty hack to hide the issue until another way appears. But as said, if this is intended...

@fishka666

stupid or not, this is work. you criticize, but offered nothing.

@yueh
Member
yueh commented May 29, 2016

It does not work or fix the problem and anyone with a good understanding of AE2 internals will confirm this.

It might appear as working for your testsetup, because it has no autocrafting and only a single drive. But for anyone with more than 1 drive or any autocrafting it will break their network depending on the order it traverses the network.

Also it might still allow duping of fluids or items via other means.
Say try a loop with an export bus and validate, if this does not bypass it under some unknown circumstances.

@fishka666

It does not work or fix the problem

??? Don't fix problem???? What you mean? Was DUPE -> NO DUPE = Problem FIXED!

@fishka666
fishka666 commented May 29, 2016 edited

I find better way ;)

public void remove( GridNode gridNode )
{
    Class<? extends IGridHost> machineClass = gridNode.getMachineClass();

    Set<IGridNode> nodes = this.machines.get( machineClass );
    if( nodes != null )
    {
        nodes.remove( gridNode );
    }

    gridNode.setGridStorage( null );

    if( this.pivot == gridNode )
    {
        Iterator<IGridNode> n = this.getNodes().iterator();
        if( n.hasNext() )
        {
            this.pivot = (GridNode) n.next();
        }
        else
        {
            this.pivot = null;
            TickHandler.INSTANCE.removeNetwork( this );
            this.myStorage.remove();
        }
    }

            for( IGridCache c : this.caches.values() )
    {
        IGridHost machine = gridNode.getMachine();
        c.removeNode( gridNode, machine );
    }

    gridNode.getGridProxy().gridChanged();
    // postEventTo( gridNode, networkChanged );

}
@yueh
Member
yueh commented May 29, 2016

I would really recommend stopping guessing and aimlessly copying code around as long as you do not fully understand then intentions behind the code.

There is a reason why the remove is done in the inverse order compared to add. Not doing it this way can cause data loss as gridnodes are no longer able to store their data at network level. For AE2 itself that might just open the possibility to dupe energy. For some addons it might result in them duping everything because they can no longer successfully save the new state.

So if your goal is to break AE: Good job.

But if you really want to help. Start actually debugging the issue at its lowest level.
Otherwise you will just introduce new bugs without actually fixing the bug and also potentially new ways to dupe.

@fishka666
fishka666 commented May 29, 2016 edited

i just give two method to fix dupe, I could care less about conflicts with mods, I only care about this dupe. I'm not trying to break ae, I just made the change, want download want no, it's just a fix for today. Perhaps the author will make normal corrections in the next versions, but what to do now? remove the mod from the server at all?

You're in the team who created this mod, and you can't fix it? why I should look at an unknown for me code and try to help other minecraft server owners? why you only criticize and don't want to admit the real problem?

@ArcasCZ
ArcasCZ commented May 29, 2016

@fishka666 is right. AE2 authors doesn't care abut this bug - I don't even know if they have time to do anythink else with this mod. So, if you don't have better idea, then stop preaching. He's doing his best. If you ever own server for more people, that you friends, you have to know, that one small duplication bug can ruin whole server economic and only one solution is to fix it - any means necessary.

@fishka666
fishka666 commented May 29, 2016 edited

he kill my links! what do people who will not be able to compile? AE with this bug can't exist on the server!

@yueh
Member
yueh commented May 29, 2016 edited

If you want to run a custom build for it, this is fine. But you need to be aware of any consequences. Which could replace one dupe with another one, because suddenly something cannot save their modified inventory correctly and will revert to their old state when added back to the network.
Maybe it results in player losing the stack of diamonds they inserted into the network, or maybe they just duped the stack they extracted be cycling something.
Should your players really not have enough self-control and cannot live without exploiting it, maybe removing AE2 is an option then. It is really your own decision.
We just provide AE more or less for free and our personal interests/fun, but really without any further obligation. If you do not like it, nobody forces you to use it.

But I simply cannot support some obviously broken builds floating around. Some users might think it is fine to use it. Until they lose data and we have to deal with it. Which binds even more of our limited time.
Also it is not really a trusted source. I do not doubt that you did not tamper with it, but I simply cannot eliminate the risk. If you would at least make it a real PR, it would go through our normal CI servers and everyone can actually look at the code changes before trying it.
So please bear with me here, if things are not as fast as you want.

I probably suspect GridStorageCache.java#L91-L93 being a potential issue. The MENetworkCellArrayUpdate should issue a full rebuild of all storage cells. Mabye the event needs to be actually sent last, maybe not, or maybe even twice? Or maybe the drive needs to send it when being removed? Or maybe it is just a sync issue between the server and terminal? Or maybe even two completely different bugs, the one @bookerthegeek found and some additional introduced by bukkit and handling tile entities differently. Mainly as the ways to reproduce are pretty different.

As long as nobody has the time or is able to attach a breakpoint to TileDrive#invalidate() and step through it, it is simply just guessing without providing a real fix and we might have the same issue again in a couple of days or weeks.

But it is simply not done in 5 minutes.
I probably have to at least schedule half a work day (4h) just to have reliable way to reproduce/debug and hopefully track it down during this time. Then afterwards find a solution, which does not break all addons or introduces new bugs/dupes/etc. As well as finally testing it and not just the "works for me, I do not care about anyone else" testing.
Together this can easily be a full work day, maybe even more. Something I currently cannot schedule at all, especially as it really needs a continouos time frame

Edit
@fishka666 Please stop posting the links, I can understand your desire to share a fixed build and I really want to provide a solution, too. But giving some random person an potentially untrusted/broken download is a risk I do not want to take. Also will you support any potential issue coming up from now on, because it might be your build who caused it? We simply cannot without knowing exactly what it does.

@fishka666
fishka666 commented May 29, 2016 edited

this is a very serious BUG, and this mod cant work on the servers with this problem ( its very simple dupe, very simple) ! You tell about new problems with my fix, but you're wrong, the mod do not working at the moment, it's better this way or it should be removed from all servers on the planet )))!

But giving some random person an potentially untrusted/broken download is a risk I do not want to take.

But you can check it.

@XFactHD
Collaborator
XFactHD commented May 30, 2016

@fishka666 I don't get where the difference is between having this particular dupe bug in the mod or another dupe bug introduced by a "quick-and-ugly" fix, could you explain that to me please?

@fishka666
fishka666 commented May 30, 2016 edited

don't care about those bugs, about which no one knows, but this bug killed the economy on two my servers, because almost everyone knows. if your country is not each player knows about it, you are lucky. for me this mod is dead until it is fixed! and "quick-and-ugly" better than nothing! or do you propose to pay money (who sell the same fix) ? this you actually probably thought.

@ArcasCZ
ArcasCZ commented May 30, 2016

He's right! Why he can't send link if he tell people it's potentialy nsafe and can harm server in some way? If they'll want to risk it, it's their decision.

@XFactHD
Collaborator
XFactHD commented May 30, 2016 edited

@fishka666 trust me, that is NOT how efficient software development works. A software company would immediately fire you for such thinking.
@ArcasCZ While you are right that it is the user's decision if he wants to use a potentialy unsafe/harmful unofficial version, there will definitely be many dumb people out there that will yell at the AE2 devs because of bugs/crashes/other harm caused by an unofficial version made by someone who was just trying to be nice and helpful. It is just not worth the risk.

EDIT: Also, thinking about paying money for a fix in an OpenSource project is just dumb because it would defeat the purpose of the software being OpenSource.

@bookerthegeek
Collaborator
bookerthegeek commented Jun 1, 2016 edited

@yueh, thank you for the suggestion on where to start looking. I can definitely see now that I think about it that just closing the gui would only bandaid this.

I'll start looking this weekend for a fix.

Also, just to CYA, it is ok to privatly distribute test builds until we can make a pull request?

@Wykks
Wykks commented Jun 3, 2016

EDIT: Also, thinking about paying money for a fix in an OpenSource project is just dumb because it would defeat the purpose of the software being OpenSource.

Oh really ?
https://www.bountysource.com/

@XFactHD
Collaborator
XFactHD commented Jun 3, 2016

I didn't mean those sites where you can donate to someone if you enjoy his content, I meant that it defeats the purpose of open source software if the software or fixes for it are sold for money.

@bookerthegeek
Collaborator

@XFactHD Have you heard of Linux?

@vallard192

I'm not sure it matters, the debate on if and how much money belongs in
open source doesn't help understand the underlying problem or identify a
solution to fix it.

On Sat, Jun 4, 2016 at 1:37 AM BookerTheGeek notifications@github.com
wrote:

@XFactHD https://github.com/XFactHD Have you heard of Linux?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#2276 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AIYLG-pMWtoAv28lh2qPNZdRpxLPnp-wks5qIQ8EgaJpZM4Icaxl
.

@XFactHD
Collaborator
XFactHD commented Jun 4, 2016

@bookerthegeek yes I have.

@fishka666

how about fix? any news?

@iKaneki-Ken
iKaneki-Ken commented Jun 22, 2016 edited

These things take time @fishka666,trying to rush things only cause more issues. Not only that, the developers of this mod have lives as well, @yueh has already stated that he has been extremely busy for the last little while. While I agree that this needs to be fixed, panicking flailing around with "possible" fixes will quite possibly cause more issues than it fixes. And yes, I know you said that it didn't "seem" to break anything, sometimes the most game breaking bugs are caused by the type of panic fixes that you've been proposing, maybe not right away, but in time they do show their face.

@yueh yueh added a commit that referenced this issue Nov 26, 2016
@yueh yueh Fixes #2655, #2276: Two dupe bugs related to network storage handling
* Fixes #2655: Actually remove an ICellContainer before updating the list.
* Fixes #2276: Apply tracker changes in the correct order.
d11d6e7
@yueh yueh added a commit that closed this issue Nov 26, 2016
@yueh yueh Fixes #2655, #2276: Two dupe bugs related to network storage handling
* Fixes #2655: Actually remove an ICellContainer before updating the list.
* Fixes #2276: Apply tracker changes in the correct order.
d11d6e7
@yueh yueh closed this in d11d6e7 Nov 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment