💬 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.

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269461760)
Changed to use `std::abort()` in the destructor. Reverted to the original interface:

```cpp
{
ASSERT_DEBUG_LOG("expected message");
produce log messages;
}
```

The change was necessary in order to pass the timeout to the "final check or abort" function which was the destructor and there is no way to pass parameters to the destructor. Anyway, now I pass the timeout to the constructor, save it in a member variable and use it in the destructor. With the wait it is now:

```cpp
...

💬 theuni commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1643929479)
Ping @achow101.

Being unable to build with `-O0` is not a big deal.

However, that means we're relying on compiler optimizations to reduce the code size first in order for tools to be able to process them with sane defaults. Breaking up `wallet.cpp` somewhat is prudent, imo.

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

> > This causes warnings when compiling for 32 bit?
>
> @hebasto did you end up looking into this?

I apologise for forgetting to mention that #28059 addresses that warnings.

💬 darosior commented on pull request "descriptor: do not parse from script unspendable Miniscript descriptors":
(https://github.com/bitcoin/bitcoin/pull/28112#issuecomment-1643932769)
Closing this, sorry for the noise. I don't think it is useful to forbid parsing a Miniscript descriptor from Script since it cannot be a footgun for end user and it's always better to parse a Miniscript, even though unspendable, than a raw Script to be displayed in some utilities (such as `decodescript` for instance).