💬 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-1446104867)
Ok, thank you. It was running all tests in parallel in valgrind, which may explain why things went slow.

✅ MarcoFalke closed an 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)

👍 stickies-v approved a pull request: "Handle invalid hex encoding in ParseHex"
(https://github.com/bitcoin/bitcoin/pull/25227)
ACK fad0c892c34c30cf8f50e832425210e24d45837e

An API that fails on invalid input instead of accepting the partial valid part is more robust imo. I don't see any places where this behaviour change will cause issues, but there are a lot of callsites.

---

Looking at the callsites of `ParseHex()`, it looks like there's a significant number of places where we first check `IsHex()` and then `ParseHex()`, iterating over the string twice when I think this can just be replaced with a single `TryP
...

💬 stickies-v commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#discussion_r1118568346)
nit: for all these, would prefer having consistency between using a temporary var for the `ParseHex` and `TryParseHex` tests
```suggestion
BOOST_CHECK_EQUAL(ParseHex(with_embedded_null), 0);
```

💬 stickies-v commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#discussion_r1118546663)
nit: add description of expected behaviour (and in 0000509239d4e699f57b392531f242ad6933c982 it would be "Truncated input is ignored")
```suggestion
// Truncated input makes the parsing fail entirely
```

💬 MarcoFalke commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#discussion_r1118650899)
Can you explain this?

Currently it is consistent in that a all cases use `result = ParseHex(...)`. This is the case before this pull request and not changed.

💬 MarcoFalke commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#discussion_r1118651283)
(Also, your suggestion wouldn't compile as is, I am pretty sure)

💬 stickies-v commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#discussion_r1118658103)
The below diff captures my entire suggestion (and compiles/tests). In these locations, we use a temp var for `ParseHex` but not for `TryParseHex`. It's just a style/minor thing (hence the nit), but I think having consistency between both makes it easier for the reviewer to see the similarities between both tests.

<details>
<summary>git diff</summary>

```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 65468a113..74e151b99 100644
--- a/src/test/util_tests.cpp
+
...

💬 MarcoFalke commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1446262046)
force pushed to change tests. can be re-reviewed with range-diff

💬 MarcoFalke commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#discussion_r1118687591)
Thanks, done.

💬 MarcoFalke commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#discussion_r1118687793)
thanks, done

💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1118693568)
Also ran both variants in nanobench and there is no difference.

🚀 glozow merged a pull request: "contrib: Improve verify-commits.py to work with maintainers leaving"
(https://github.com/bitcoin/bitcoin/pull/27058)

💬 hebasto commented on pull request "Translatability fixups":
(https://github.com/bitcoin-core/gui/pull/599#issuecomment-1446332571)
> Concept ACK.

> Concept ACK and initial code review ACK [5e23dab](https://github.com/bitcoin-core/gui/commit/5e23dabf2651e49a77954609c303bfc4478ff8f1)

@luke-jr

Mind rebasing this PR before [soft translation string freeze](https://github.com/bitcoin/bitcoin/issues/26549)?

💬 MarcoFalke commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1446339062)
This will break the script on all operating systems except the rolling ones like rawhide and tumbleweed?

See also:

subprocess.CalledProcessError: Command '['git', 'merge-tree', 'be2e748f378fc9ed40593a723dd18f2528705956', '14fac808bd6c12bce121011bbf50501960c7326f']' returned non-zero exit status 129.

https://cirrus-ci.com/task/4961884550463488?logs=lint#L242

👍 stickies-v approved a pull request: "Handle invalid hex encoding in ParseHex"
(https://github.com/bitcoin/bitcoin/pull/25227)
re-ACK faab273e060d27e166b5fb7fe7692614ec9e5c76

💬 furszy commented on pull request "wallet: finish addressbook encapsulation":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1446361158)
Thanks @rserranon.

> why not encapsulate the other calls to wallet->m_address_book[addr_pair.first].purpose = purpose ?

Good catch. I did this changes prior the wallet migration PR merge. Will add those too and make the addressbook member private so we don't keep adding more in the future.

💬 glozow commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1446365757)
iiuc reverting 5497c1483097a9b582ef78089a2ce1101b7d722e would make it go away though keeping `merge-tree` would be nice.
Is bumping the lint container to something that gets 2.38 an option for CI?

💬 MarcoFalke commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1446372436)
Yeah, should be easy to bump CI to `ubuntu:devel`. Though, I wonder what users are supposed to do?

💬 furszy commented on pull request "wallet: finish addressbook encapsulation":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1118780133)
done

💬 fanquake commented on pull request "I2P network optimizations":
(https://github.com/bitcoin/bitcoin/pull/26837#issuecomment-1446389184)
Added to #26878 for backporting to 24.x.