📝 fanquake opened a pull request: "doc: correct Fedora systemtap dep"
(https://github.com/bitcoin/bitcoin/pull/28110)

⚠️ hebasto opened an issue: ""missing operand" assembler warnings on Windows"
(https://github.com/bitcoin/bitcoin/issues/28111)
On the master branch @673acab223c0f896767b1ae784659df9f95452ae on Ubuntu 23.04:

```
$ make -C depends HOST=x86_64-w64-mingw32 NO_QT=1 DEBUG=1 NO_HARDEN=1
$ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site
$ make
...
CXX crypto/libbitcoin_crypto_base_la-aes.lo
CXX crypto/libbitcoin_crypto_base_la-chacha_poly_aead.lo
CXX crypto/libbitcoin_crypto_base_la-chacha20.lo
CXX crypto/libbitcoin_crypto_base_la-hkdf_sha256_32.lo
CXX cry
...

🤔 stickies-v reviewed a pull request: "rfc: Type-safe transaction identifiers"
(https://github.com/bitcoin/bitcoin/pull/28107#pullrequestreview-1538994126)
Concept ACK.

We don't consistently have clear variable naming (e.g. the ambiguous `hash`; or `txid` being used to refer to a wtxid), sometimes requiring quite a bit of developer time to figure out the expected ~type. I have experienced this quite a bit myself.

Introducing types makes this explicit at relatively low overhead, and I think the approach of subclassing `uint256` makes sense.

We'll have to be careful for this not to lead to unnecessary refactoring code churn, but that shouldn
...

💬 fanquake commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1643712997)
I don't understand what the actual problem is, or what is broken? Can you elaborate?

💬 hebasto commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1643715980)
> I don't understand what the actual problem is, or what is broken? Can you elaborate?

I'm not an expert in assembly code, but "missing operand' sounds scary.

I don't know whether this warning is safe to ignore.

👍 hebasto approved a pull request: "doc: correct Fedora systemtap dep"
(https://github.com/bitcoin/bitcoin/pull/28110#pullrequestreview-1539020657)
ACK 12edf7b155685c0e1a401cfa71ef28784362689e, tested on Fedora 38:

- with`systemtap` installed:
```
$ ./configure
...
USDT tracing = no
...
```

- with`systemtap-sdt-devel` installed:
```
$ ./configure
...
USDT tracing = yes
...
```

💬 fanquake commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1643816890)
Probably needs some actual investigation then:
i.e what code + combination of compiler flags is causing the issue, produce a minimal test case.
Is this a "new" issue, or have the warnings existed in our codebase "forever".
Is it related to the fact that you're using ubuntu mantic (devel) and/or compiler/binutils versions etc.

🚀 fanquake merged a pull request: "contrib: move user32.dll from bitcoind.exe libs"
(https://github.com/bitcoin/bitcoin/pull/28099)

💬 glozow commented on pull request "rfc: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1643837866)
I've definitely written/seen bugs that could be prevented if Txid and Wtxid were different types. When I review code it's something that I look for and it comes up surprisingly often. Interfaces taking `uint256` often implicitly expect them to be a txid, eg #23325.

Agree that checking at compile time would be nice and probably prevent bugs / make review easier. Concept ACK assuming we're ok with the amount of code change.

💬 dergoegge commented on pull request "rfc: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1643838829)
> We'll have to be careful for this not to lead to unnecessary refactoring code churn, but that shouldn't be a reason not to make this change imo.

In terms of refactoring, this should be similar to the time type-safety PRs in that we can gradually convert our code.

🚀 fanquake merged a pull request: "util: Show descriptive error messages when FileCommit fails"
(https://github.com/bitcoin/bitcoin/pull/26654)

💬 MarcoFalke commented on pull request "test: Ignore UTF-8 errors in assert_debug_log":
(https://github.com/bitcoin/bitcoin/pull/28035#issuecomment-1643863154)
Split into more commits

💬 glozow commented on pull request "Use `int32_t` type for most transaction size/weight values":
(https://github.com/bitcoin/bitcoin/pull/23962#issuecomment-1643870289)
> This causes warnings when compiling for 32 bit?

@hebasto did you end up looking into this?

💬 darosior commented on pull request "Descriptors: rule out unspendable miniscript descriptors":
(https://github.com/bitcoin/bitcoin/pull/27997#issuecomment-1643890926)
I forgot also checking this when parsing from Script..

🤔 darosior requested changes to a pull request: "descriptors: do not return top-level only funcs as sub descriptors"
(https://github.com/bitcoin/bitcoin/pull/28067#pullrequestreview-1539198512)
Concept ACK. Good catch. However i think you reintroduce the same bug in the second commit by returning early as `raw()` when in the first case it should be an address descriptor and the second it should fail if not at top-level.

<details>

<summary> This diff fixes it: </summary>

```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 9e4f775f41..09ded5fc61 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1662,12 +1662,7 @@ std::un
...

💬 darosior commented on pull request "descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#discussion_r1269419796)
Same here but it's also incorrect because we're not necessarily at top level in this branch. This could create a `sh(raw())` which would be invalid.

💬 darosior commented on pull request "descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#discussion_r1269418215)
This is unnecessary? It would be returned as such at the end of the function. And not as `raw()` as in your test, but as an actual address.

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1643919170)
`612ba17fca...55c84c2d3b`: rebase due to conflicts plus reduce the changes to `DebugLogHelper`: https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1263332063.

📝 darosior opened a pull request: "descriptor: do not parse from script unspendable Miniscript descriptors"
(https://github.com/bitcoin/bitcoin/pull/28112)
https://github.com/bitcoin/bitcoin/pull/27997 but for Script parsing.

I've added a test demonstrating the behaviour despite the descriptor parsing also currently being incorrect (`wsh(raw())`, see #28067). I guess whichever comes after the other between this PR and #28067 would be trivial to rebase on to fix it.

💬 darosior commented on pull request "descriptor: do not parse from script unspendable Miniscript descriptors":
(https://github.com/bitcoin/bitcoin/pull/28112#issuecomment-1643924292)
Actually i'm not even sure we'd like to do this, as i don't see how it could be a footgun for a user.