Skip to content

Commit

Permalink
[nis]: block lessor is not set correctly in database in all cases
Browse files Browse the repository at this point in the history
 problem: when (future) remote link is pending fixLessor
          findForwardedStateByAddress will use the future (not active) remote link for resolution
solution: update RemoteLinks to retrieve active (not current) remote link
          use height already passed into findForwardedStateByAddress to choose active link
  • Loading branch information
Jaguar0625 committed Aug 21, 2024
1 parent 9ca1d3c commit fd4b4ae
Show file tree
Hide file tree
Showing 7 changed files with 194 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,11 @@ public AccountState findLatestForwardedStateByAddress(final Address address) {
public AccountState findForwardedStateByAddress(final Address address, final BlockHeight height) {
final AccountState state = this.findStateByAddress(address);
final ReadOnlyRemoteLinks remoteLinks = state.getRemoteLinks();
if (!remoteLinks.isRemoteHarvester()) {
if (!remoteLinks.isRemoteHarvesterAt(height)) {
return state;
}

final RemoteLink remoteLink = remoteLinks.getCurrent();
final RemoteLink remoteLink = remoteLinks.getActive(height);
final long settingHeight = height.subtract(remoteLink.getEffectiveHeight());
final int remoteHarvestingDelay = NemGlobals.getBlockChainConfiguration().getBlockChainRewriteLimit();
boolean shouldUseRemote = false;
Expand Down
27 changes: 26 additions & 1 deletion nis/src/main/java/org/nem/nis/state/ReadOnlyRemoteLinks.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,52 @@ public interface ReadOnlyRemoteLinks {
boolean isEmpty();

/**
* Gets a value indicating whether or the owning account has enabled remote harvesting and is forwarding its harvesting power to a
* Gets a value indicating whether or not the owning account has enabled remote harvesting and is forwarding its harvesting power to a
* remote account.
*
* @return true if the owning account has enabled remote harvesting.
*/
boolean isHarvestingRemotely();

/**
* Gets a value indicating whether or not the owning account has enabled remote harvesting and is forwarding its harvesting power to a
* remote account at the specified block height.
*
* @param height The block height.
* @return true if the owning account has enabled remote harvesting at the specified block height.
*/
boolean isHarvestingRemotelyAt(final BlockHeight height);

/**
* Gets a value indicating whether or not the owning account is a remote harvester account.
*
* @return true if the owning account is a remote harvester.
*/
boolean isRemoteHarvester();

/**
* Gets a value indicating whether or not the owning account is a remote harvester account at the specified block height.
*
* @param height The block height.
* @return true if the owning account is a remote harvester at the specified block heighte.
*/
boolean isRemoteHarvesterAt(final BlockHeight height);

/**
* Gets the current remote link.
*
* @return The current remote link.
*/
RemoteLink getCurrent();

/**
* Gets the remote link active at the specified block height.
*
* @param height The block height.
* @return The active remote link at the specified block height.
*/
RemoteLink getActive(final BlockHeight height);

/**
* Gets the remote status at the specified block height.
*
Expand Down
33 changes: 29 additions & 4 deletions nis/src/main/java/org/nem/nis/state/RemoteLinks.java
Original file line number Diff line number Diff line change
Expand Up @@ -57,29 +57,54 @@ public boolean isHarvestingRemotely() {
return !this.isEmpty() && RemoteLink.Owner.HarvestingRemotely == this.remoteLinks.peek().getOwner();
}

@Override
public boolean isHarvestingRemotelyAt(final BlockHeight height) {
final RemoteLink activeRemoteLink = this.getActive(height);
return null != activeRemoteLink && RemoteLink.Owner.HarvestingRemotely == activeRemoteLink.getOwner();
}

@Override
public boolean isRemoteHarvester() {
return !this.isEmpty() && RemoteLink.Owner.RemoteHarvester == this.remoteLinks.peek().getOwner();
}

@Override
public boolean isRemoteHarvesterAt(final BlockHeight height) {
final RemoteLink activeRemoteLink = this.getActive(height);
return null != activeRemoteLink && RemoteLink.Owner.RemoteHarvester == activeRemoteLink.getOwner();
}

@Override
public RemoteLink getCurrent() {
return this.isEmpty() ? null : this.remoteLinks.peek();
}

@Override
public RemoteLink getActive(final BlockHeight height) {
RemoteLink activeRemoteLink = null;
for (final RemoteLink remoteLink : this.remoteLinks) {
if (remoteLink.getEffectiveHeight().compareTo(height) < 1) {
activeRemoteLink = remoteLink;
}
}

return activeRemoteLink;
}

@Override
public RemoteStatus getRemoteStatus(final BlockHeight height) {
if (this.isEmpty()) {
final RemoteLink activeRemoteLink = this.getActive(height);
if (null == activeRemoteLink) {
return RemoteStatus.NOT_SET;
}

// currently we can only have Activate and Deactivate, so we're ok to use single boolean for this

final boolean isActivated = ImportanceTransferMode.Activate == this.getCurrent().getMode();
final long heightDiff = height.subtract(this.getCurrent().getEffectiveHeight());
final boolean isActivated = ImportanceTransferMode.Activate == activeRemoteLink.getMode();
final long heightDiff = height.subtract(activeRemoteLink.getEffectiveHeight());
final boolean withinLimit = heightDiff < NemGlobals.getBlockChainConfiguration().getBlockChainRewriteLimit();

if (this.isHarvestingRemotely()) {
if (this.isHarvestingRemotelyAt(height)) {
if (isActivated) {
return withinLimit ? RemoteStatus.OWNER_ACTIVATING : RemoteStatus.OWNER_ACTIVE;
} else {
Expand Down
19 changes: 19 additions & 0 deletions nis/src/test/java/org/nem/nis/cache/AccountStateCacheTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,25 @@ private boolean isLocalState(final ImportanceTransferMode mode, final int remote
return forwardedState.equals(state);
}

@Test
public void findForwardedStateByAddressIsHeightAware() {
// Arrange:
final Address address = Utils.generateRandomAddress();
final T cache = this.createCacheWithoutAutoCache();
final AccountState state = this.addToCache(cache.copy(), address);

final RemoteLink link1 = new RemoteLink(Utils.generateRandomAddress(), new BlockHeight(1000), ACTIVATE, RemoteLink.Owner.RemoteHarvester);
final RemoteLink link2 = new RemoteLink(Utils.generateRandomAddress(), new BlockHeight(2000), ACTIVATE, RemoteLink.Owner.HarvestingRemotely);
state.getRemoteLinks().addLink(link1);
state.getRemoteLinks().addLink(link2);

// Act:
final AccountState forwardedState = cache.findForwardedStateByAddress(address, new BlockHeight(1500));

// Assert: second link is ignored because 2000 > 1500
MatcherAssert.assertThat(forwardedState.getAddress(), IsEqual.equalTo(link1.getLinkedAddress()));
}

// endregion

// region removeFromCache
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,61 @@ public void processBlockUpdatesBlockLessor() {
MatcherAssert.assertThat(block.getLessor(), IsEqual.equalTo(account));
}

@Test
public void processBlockUpdatesBlockLessorCorrectlyWhenFutureRemoteLinksArePending() {
// Arrange:
final ForkConfiguration forkConfiguration = new ForkConfiguration.Builder().build();
final SynchronizedAccountStateCache accountStateCache = new SynchronizedAccountStateCache(new DefaultAccountStateCache());
final DefaultNisCache nisCache = new DefaultNisCache(new SynchronizedAccountCache(new DefaultAccountCache()), accountStateCache,
new SynchronizedPoxFacade(new DefaultPoxFacade(NisUtils.createImportanceCalculator())),
new SynchronizedHashCache(new DefaultHashCache()),
new SynchronizedNamespaceCache(new DefaultNamespaceCache(forkConfiguration.getMosaicRedefinitionForkHeight())));
final RealBlockChainTestContext context = new RealBlockChainTestContext(nisCache);

// Setup remote harvesting, such that
// a. remote is active from start of block chain
// b. remote is deactivating at initial block height (and next block height)
// c. remote is (re)activating at initial block height + 1000
final Account account = context.createAccount(Amount.fromNem(100000));
final Account remoteAccount = context.createAccount(Amount.ZERO);

final RemoteLink remoteLink1a = new RemoteLink(remoteAccount.getAddress(), BlockHeight.ONE, ImportanceTransferMode.Activate,
RemoteLink.Owner.HarvestingRemotely);
final RemoteLink remoteLink1b = new RemoteLink(remoteAccount.getAddress(), context.getInitialBlockHeight(),
ImportanceTransferMode.Deactivate, RemoteLink.Owner.HarvestingRemotely);
final RemoteLink remoteLink1c = new RemoteLink(remoteAccount.getAddress(), new BlockHeight(context.getInitialBlockHeight().getRaw() + 100),
ImportanceTransferMode.Activate, RemoteLink.Owner.HarvestingRemotely);
final AccountState accountState = accountStateCache.findStateByAddress(account.getAddress());
accountState.getRemoteLinks().addLink(remoteLink1a);
accountState.getRemoteLinks().addLink(remoteLink1b);
accountState.getRemoteLinks().addLink(remoteLink1c);

final RemoteLink remoteLink2a = new RemoteLink(account.getAddress(), BlockHeight.ONE, ImportanceTransferMode.Activate,
RemoteLink.Owner.RemoteHarvester);
final RemoteLink remoteLink2b = new RemoteLink(account.getAddress(), context.getInitialBlockHeight(),
ImportanceTransferMode.Deactivate, RemoteLink.Owner.RemoteHarvester);
final RemoteLink remoteLink2c = new RemoteLink(account.getAddress(), new BlockHeight(context.getInitialBlockHeight().getRaw() + 100),
ImportanceTransferMode.Activate, RemoteLink.Owner.RemoteHarvester);
final AccountState remoteAccountState = accountStateCache.findStateByAddress(remoteAccount.getAddress());
remoteAccountState.getRemoteLinks().addLink(remoteLink2a);
remoteAccountState.getRemoteLinks().addLink(remoteLink2b);
remoteAccountState.getRemoteLinks().addLink(remoteLink2c);

final Block block = context.createNextBlock(remoteAccount);
block.sign();

// Act:
final ValidationResult processResult = context.processBlock(block);

// Sanity:
MatcherAssert.assertThat(block.getHeight(), IsEqual.equalTo(context.getInitialBlockHeight().next()));

// Assert:
MatcherAssert.assertThat(processResult, IsEqual.equalTo(ValidationResult.SUCCESS));
MatcherAssert.assertThat(block.getLessor(), IsNull.notNullValue());
MatcherAssert.assertThat(block.getLessor(), IsEqual.equalTo(account));
}

// endregion

// region exploitRaceConditionBetweenBlockChainAndNewBlockTransactionGathering
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ private void saveBlock(final Block block) {
this.blockChainLastBlockLayer.analyzeLastBlock(dbBlock);
}

public BlockHeight getInitialBlockHeight() {
return this.initialBlockHeight;
}

// region mapping helpers

/**
Expand Down
59 changes: 59 additions & 0 deletions nis/src/test/java/org/nem/nis/state/RemoteLinksTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,65 @@ private static void assertRemoteStatus(final BlockHeight height, final BlockHeig

// endregion

// region height dependent accessors

@Test
public void canAccessRemoteLinkAtHeight() {
// Arrange:
final RemoteLinks links = new RemoteLinks();

// Act:
final RemoteLink link1 = RemoteLinkFactory.activateRemoteHarvester(Utils.generateRandomAddress(), new BlockHeight(7));
final RemoteLink link2 = RemoteLinkFactory.activateHarvestingRemotely(Utils.generateRandomAddress(), new BlockHeight(9));
links.addLink(link1);
links.addLink(link2);

// Assert:
MatcherAssert.assertThat(links.isEmpty(), IsEqual.equalTo(false));

// - isHarvestingRemotelyAt is height dependent
MatcherAssert.assertThat(links.isHarvestingRemotelyAt(new BlockHeight(6)), IsEqual.equalTo(false));
MatcherAssert.assertThat(links.isHarvestingRemotelyAt(new BlockHeight(7)), IsEqual.equalTo(false));
MatcherAssert.assertThat(links.isHarvestingRemotelyAt(new BlockHeight(8)), IsEqual.equalTo(false));
MatcherAssert.assertThat(links.isHarvestingRemotelyAt(new BlockHeight(9)), IsEqual.equalTo(true));
MatcherAssert.assertThat(links.isHarvestingRemotelyAt(new BlockHeight(10)), IsEqual.equalTo(true));

// - isRemoteHarvesterAt is height dependent
MatcherAssert.assertThat(links.isRemoteHarvesterAt(new BlockHeight(6)), IsEqual.equalTo(false));
MatcherAssert.assertThat(links.isRemoteHarvesterAt(new BlockHeight(7)), IsEqual.equalTo(true));
MatcherAssert.assertThat(links.isRemoteHarvesterAt(new BlockHeight(8)), IsEqual.equalTo(true));
MatcherAssert.assertThat(links.isRemoteHarvesterAt(new BlockHeight(9)), IsEqual.equalTo(false));
MatcherAssert.assertThat(links.isRemoteHarvesterAt(new BlockHeight(10)), IsEqual.equalTo(false));

// - getActive is height dependent
MatcherAssert.assertThat(links.getActive(new BlockHeight(6)), IsEqual.equalTo(null));
MatcherAssert.assertThat(links.getActive(new BlockHeight(7)), IsEqual.equalTo(link1));
MatcherAssert.assertThat(links.getActive(new BlockHeight(8)), IsEqual.equalTo(link1));
MatcherAssert.assertThat(links.getActive(new BlockHeight(9)), IsEqual.equalTo(link2));
MatcherAssert.assertThat(links.getActive(new BlockHeight(10)), IsEqual.equalTo(link2));
}

@Test
public void canAccessRemoteStatusAtHeight() {
// Arrange:
final RemoteLinks links = new RemoteLinks();

// Act:
final RemoteLink link1 = RemoteLinkFactory.activateRemoteHarvester(Utils.generateRandomAddress(), new BlockHeight(7));
final RemoteLink link2 = RemoteLinkFactory.activateHarvestingRemotely(Utils.generateRandomAddress(), new BlockHeight(9));
links.addLink(link1);
links.addLink(link2);

// Assert: getRemoteStatus uses *active* remote link
MatcherAssert.assertThat(links.getRemoteStatus(new BlockHeight(6)), IsEqual.equalTo(RemoteStatus.NOT_SET));
MatcherAssert.assertThat(links.getRemoteStatus(new BlockHeight(7)), IsEqual.equalTo(RemoteStatus.REMOTE_ACTIVATING));
MatcherAssert.assertThat(links.getRemoteStatus(new BlockHeight(8)), IsEqual.equalTo(RemoteStatus.REMOTE_ACTIVATING));
MatcherAssert.assertThat(links.getRemoteStatus(new BlockHeight(9)), IsEqual.equalTo(RemoteStatus.OWNER_ACTIVATING));
MatcherAssert.assertThat(links.getRemoteStatus(new BlockHeight(10)), IsEqual.equalTo(RemoteStatus.OWNER_ACTIVATING));
}

// endregion

// region copy

@Test
Expand Down

0 comments on commit fd4b4ae

Please sign in to comment.