Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merge #15334, #13076, #15266, #13128 #4420

Merged
merged 4 commits into from Sep 16, 2021
Merged

Merge #15334, #13076, #15266, #13128 #4420

merged 4 commits into from Sep 16, 2021

Conversation

vijaydasmp
Copy link

@vijaydasmp vijaydasmp commented Sep 13, 2021 •
edited by UdjinM6

No description provided.

@vijaydasmp vijaydasmp marked this pull request as draft September 13, 2021 04:17
@laanwj @vijaydasmp
Merge bitcoin#15334: wallet: Log absolute paths for the wallets
1efb500 
a4b92e4 Log full paths for wallets (Hennadii Stepanov)

Pull request description:

  Fix bitcoin#15333

  `debug.log` with this PR:
  ```
  ...
  2019-02-03T19:02:35Z Using wallet directory /home/hebasto/.bitcoin/testnet3/wallets
  2019-02-03T19:02:35Z init message: Verifying wallet(s)...
  2019-02-03T19:02:35Z Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
  2019-02-03T19:02:35Z Using wallet test_alpha/wallet.dat
  2019-02-03T19:02:35Z BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/test_alpha/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/test_alpha/db.log
  2019-02-03T19:02:35Z Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
  2019-02-03T19:02:35Z Using wallet alpha_wallet/wallet.dat
  2019-02-03T19:02:35Z BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/alpha_wallet/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/alpha_wallet/db.log
  2019-02-03T19:02:35Z Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
  2019-02-03T19:02:35Z Using wallet wallet.dat
  2019-02-03T19:02:35Z BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/db.log
  2019-02-03T19:02:35Z Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
  2019-02-03T19:02:35Z Using wallet none/wallet.dat
  2019-02-03T19:02:35Z BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/none/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/none/db.log
  2019-02-03T19:02:35Z init message: Loading banlist...
  ...
  ```

Tree-SHA512: 8dd4408d3f6b04f396dd0ae0d248fedc3a0f6d36788556ae1662443f06f2ecce1c2be9456bf8d1b3d25b29c2a0cfb03cb805bde0a40387e68988ab932e17e118
@vijaydasmp vijaydasmp force-pushed the backport_v18_vijay_batch3_1 branch 6 times, most recently from aa6a7a2 to b9f2bef Compare September 13, 2021 13:40
@vijaydasmp
Copy link
Author

The CI is acting flaky again
I had one good run https://gitlab.com/dashpay/dash/-/pipelines/369583089, after that, I rebased the same code changes to fixup/squash the commit into one commit and force pushed it then with code changes observed https://gitlab.com/dashpay/dash/-/pipelines/369606293 feature_llmq_data_recovery.py test failing

After that tried to add some more changes to shake the CI then some "depend" failure starts occurring which is happening even after deleting the changes,

Does Anyone else also observe this type of behavior? @UdjinM6 @PastaPastaPasta

@UdjinM6
Copy link

UdjinM6 commented Sep 13, 2021

Gitlab itself is having issues atm https://status.gitlab.com/pages/incident/5b36dc6502d06804c08349f7/613f5ad56e8609053a627730, expect CI to fail until it's resolved

@vijaydasmp
Copy link
Author

Oh I see, Thanks for updating me

@vijaydasmp vijaydasmp marked this pull request as ready for review September 14, 2021 04:00
@vijaydasmp
Copy link
Author

Hello @UdjinM6 @PastaPastaPasta please have a look

@UdjinM6 UdjinM6 changed the title Merge #15334: wallet: Log absolute paths for the wallets Merge #15334, #13076, #15266, #13128 Sep 14, 2021
UdjinM6
UdjinM6 requested changes Sep 14, 2021
View reviewed changes
src/test/test_dash.h Outdated Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
PastaPastaPasta
PastaPastaPasta reviewed Sep 14, 2021
View reviewed changes
src/test/test_dash.h Outdated
@@ -33,6 +40,7 @@ extern FastRandomContext g_insecure_rand_ctx;
*/
extern bool g_mock_deterministic_tests;


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop this newline

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @PastaPastaPasta dropped new line

src/wallet/rpcdump.cpp
Comment on lines -771 to +772
pwallet->ScanForWalletTransactions(chainActive[nStartHeight], nullptr, reserver, true);
const CBlockIndex *stop_block, *failed_block;
pwallet->ScanForWalletTransactions(chainActive[nStartHeight], nullptr, reserver, failed_block, stop_block, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these changes belong in 13076

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine, importelectrumwallet is a Dash-specific rpc.

@vijaydasmp
Copy link
Author

Thanks, @UdjinM6 for comments, Implemented all review comments

@meshcollider @vijaydasmp
Merge bitcoin#13076: Fix ScanForWalletTransactions to return an enum …
a894cb6 
…indicating scan result: success / failure / user_abort

bd3b036 Add stop_block out arg to ScanForWalletTransactions (Ben Woosley)
3002d6c Return a status enum from ScanForWalletTransactions (Ben Woosley)
bb24d68 Make CWallet::ScanForWalletTransactions args and return value const (Ben Woosley)

Pull request description:

  Return the failed block as an out arg.

  Fixes bitcoin#11450.

  /cc bitcoin#12275

Tree-SHA512: 6a523e5425ebfe24e664a942ae21c797ccc1281c25b1bf8d02ad95c19dae343fd8051985ef11853474de7628fd6bed5f15190fbc087c3466ce6fdecab37d72a9
@vijaydasmp
Copy link
Author

@PastaPastaPasta @UdjinM6, Thanks for Review comments, I have Implemented them

@UdjinM6 UdjinM6 added this to the 18 milestone Sep 15, 2021
UdjinM6
UdjinM6 previously approved these changes Sep 15, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, utACK

PastaPastaPasta
PastaPastaPasta requested changes Sep 15, 2021
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one little change

src/init.cpp Outdated
g_logger->m_file_path.string()));
LogInstance().m_file_path.string()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing the indentation changes upstream did https://github.com/bitcoin/bitcoin/pull/15266/files#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69dL1208-R1208

(Otherwise looks good)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed Indentation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why after this indentation change
rpc_setban.py test is failing

Hello @PastaPastaPasta @UdjinM6 Can someone please retrigger CI, This indentation changed should not have caused any issue or test failure, I am suspecting CI flakiness here

MarcoFalke added 2 commits September 16, 2021 07:41
@vijaydasmp
Merge bitcoin#15266: memory: Construct globals on first use
15f1480 
77777c5 log: Construct global logger on first use (MarcoFalke)

Pull request description:

  The (de)initialization order is not well defined in C++, so generally it is not safe to use globals as the (de/con)structor of one global could use the (de/con)structor of another global before/after it has been (con/de)structed.

  Specifically this fixes:
  * `g_logger` might not be initialized on the first use, so do that. (Fixes bitcoin#15111)

Tree-SHA512: eb9c22f4baf31ebc5b0b9ee6a51d1354bae1f0df186cc0ce818b4483c7b5a7f90268d2b549ee96b4c57f8ef36ab239dc6497f74f3e2ef166038f7437c368297d
@vijaydasmp
Merge bitcoin#13128: policy: Add Clang thread safety annotations for …
3dbd958 
…variables guarded by cs_feeEstimator

dae1423 Add locking annotations to feeStats, shortStats and longStats (practicalswift)
764e42f scripted-diff: Rename from cs_feeEstimator to m_cs_fee_estimator (practicalswift)
9a789d4 policy: Add Clang thread safety annotations for variables guarded by cs_feeEstimator (practicalswift)

Pull request description:

  * Add Clang thread safety annotations for variables guarded by `cs_feeEstimator`
  * ~~Add missing `cs_feeEstimator` locks~~

Tree-SHA512: 24b1d876ad53524ee8989b9658ac1a1b2766ebb3b27a1f84601d207e74d090e33738b814afac2a1f5bcd37565abcb361c6e5adae212840ff1ca32c3c42953391
@vijaydasmp
Copy link
Author

Hello @PastaPastaPasta All review comments implemented

PastaPastaPasta
PastaPastaPasta approved these changes Sep 16, 2021
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK for merging via merge commit

UdjinM6
UdjinM6 approved these changes Sep 16, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@PastaPastaPasta PastaPastaPasta merged commit 78e04b9 into dashpay:develop Sep 16, 2021
@vijaydasmp vijaydasmp deleted the backport_v18_vijay_batch3_1 branch December 29, 2021 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
18
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vijaydasmp @UdjinM6 @PastaPastaPasta @laanwj @meshcollider