Bitcoin Core Github
44 subscribers
120K links
Download Telegram
💬 ishaanam commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#issuecomment-1455354602)
> Is this a draft because you haven't tested under Valgrind yet?

Yes, it was.
> Worked for me under a recent Valgrind run.

Great, I will mark it as ready for review.
👋 ishaanam's pull request is ready for review: "test: fix race condition in encrypted wallet rescan tests"
(https://github.com/bitcoin/bitcoin/pull/27199)
⚠️ YANGHONGEUN opened an issue: "79f0a3f1f42e0421e60cf9a57f6c70c6be9221a1"
(https://github.com/bitcoin/bitcoin/issues/27207)
<!-- Needs the label "good first issue" assigned manually before or after opening -->

<!-- A good first issue is an uncontroversial issue, that has a relatively unique and obvious solution -->

<!-- Motivate the issue and explain the solution briefly -->

#### Useful skills:

<!-- (For example, “std::thread”, “Qt5 GUI and async GUI design” or “basic understanding of Bitcoin mining and the Bitcoin Core RPC interface”.) -->

#### Want to work on this issue?

For guidance on contributing, please r
...
fanquake closed an issue: "79f0a3f1f42e0421e60cf9a57f6c70c6be9221a1"
(https://github.com/bitcoin/bitcoin/issues/27207)
:lock: fanquake locked an issue: "79f0a3f1f42e0421e60cf9a57f6c70c6be9221a1"
(https://github.com/bitcoin/bitcoin/issues/27207)
fanquake closed an issue: "Hjik"
(https://github.com/bitcoin/bitcoin/issues/27206)
:lock: fanquake locked an issue: "Hjik"
(https://github.com/bitcoin/bitcoin/issues/27206)
fanquake closed an issue: "Vahhh"
(https://github.com/bitcoin/bitcoin/issues/27204)
:lock: fanquake locked an issue: "Vahhh"
(https://github.com/bitcoin/bitcoin/issues/27204)
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1126052875)
> add (a TODO for) method to the wallet tool to re-run the helper functions; this would be for advanced users who somehow ended up with a wallet not matching this pattern, but have repented, added the right descriptors, and now want the global HD key field set


Is this really needed if we add support for `sethdseed` later?
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1455748229)
utACK 1cfed6ce6cd8c9f9f1e93662ff8f92cd5a5b91c4 (also reviewed the tests)
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126088720)
this is slightly confusing because I'd expect the number to go one up from the previous case (from 3 => 4).

The test is passing, because you flipped `positive_only` and now excluded negative group is balanced by the group for `dest7`.

I'd suggest keeping `positive_only=false` and bumping the numbers
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126099320)
Commit message is now inconsistent with the code. Either drop the details of test cases or update them.
💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126122594)
The reason why I suggested this change is because it's hard to explain why we need a `insert_mixed` flag. Why don't we just always add groups to `mixed` vector? I understand that's because of how aps works, but just looking at this method it's mysterious.
💬 MarcoFalke commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1126125555)
Any reason to use the cli, when the normal rpc can be used directly?

Also, I wonder if this introduces another race where the actual rescan in `t.start()` is scheduled after this call. Avoiding this race might be ugly, but I think it could be possible by using assert_debug_log with a timeout value to sync on the rescan actually starting?
💬 TheCharlatan commented on pull request "guix: pass `--enable-initfini-array` to release GCC":
(https://github.com/bitcoin/bitcoin/pull/27153#issuecomment-1455762187)
ACK 127c637cf0a80e0ea68a7c5aaa088e5ccc9d3d13
💬 MarcoFalke commented on issue "Intermittent issue in p2p_ibd_stalling.py self.wait_until(lambda: self.total_bytes_recv_for_blocks() == 172761)":
(https://github.com/bitcoin/bitcoin/issues/27112#issuecomment-1455762739)
Looks like a different issue, so maybe open a new one? Also, we should probably increase the default timeout when `--valgrind`
⚠️ fanquake opened an issue: "Issue in `p2p_ibd_stalling.py` under Valgrind"
(https://github.com/bitcoin/bitcoin/issues/27208)
At 40c6c85c05812ee8bf824b639307b1ac17a001c4 with the native_valgrind job:
```bash
test 2023-03-05T21:26:43.074000Z TestFramework.node0 (DEBUG): Connecting to 127.0.0.1:12173 outbound-full-relay
node0 2023-03-05T21:26:43.265731Z [msghand] [net_processing.cpp:5807] [SendMessages] [net] Requesting block 752405439cea869d584044084502582bc209e4ef97e4bf3b8c2ba3958acaf606 (21) peer=0
node0 2023-03-05T21:26:43.267295Z [msghand] [net.cpp:2816] [PushMessage] [net] sending getdata (37 bytes) peer=
...
💬 fanquake commented on issue "Intermittent issue in p2p_ibd_stalling.py self.wait_until(lambda: self.total_bytes_recv_for_blocks() == 172761)":
(https://github.com/bitcoin/bitcoin/issues/27112#issuecomment-1455783529)
> Looks like a different issue, so maybe open a new one?

#27208
💬 MarcoFalke commented on issue "Issue in `p2p_ibd_stalling.py` under Valgrind":
(https://github.com/bitcoin/bitcoin/issues/27208#issuecomment-1455785923)
What did you set the timeout factor to? Is there a combined log or similar?
👍 TheCharlatan approved a pull request: "wallet: use Mutex for g_sqlite_mutex instead of GlobalMutex"
(https://github.com/bitcoin/bitcoin/pull/25491)
ACK 4163093d6374e865dc57e33dbea0e7e359062e0a